[PATCH v2 00/11] Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts

[PATCH v2 00/11] Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts

[Théo Lebrun]

Hi,

This series adds two tangent features to the Nomadik I2C controller:

 - Add a new compatible to support Mobileye EyeQ5 which uses the same IP
   block as Nomadik.

   It has two quirks to be handled:
    - The memory bus only supports 32-bit accesses. Avoid readb() and
      writeb() calls that might generate byte load/store instructions.
    - We must write a value into a shared register region (OLB)
      depending on the I2C bus speed.

 - Allow xfer timeouts below one jiffy by using a waitqueue and hrtimers
   instead of a completion.

   The situation to be addressed is:
    - Many devices on the same I2C bus.
    - One xfer to each device is sent at regular interval.
    - One device gets stuck and does not answer.
    - With long timeouts, following devices won't get their message. A
      shorter timeout ensures we can still talk to the following
      devices.

   This clashes a bit with the current i2c_adapter timeout field that
   stores a jiffies amount. We therefore avoid it and store the value
   in a private struct field, as a µs amount. If the timeout is less
   than a jiffy duration, we switch from standard jiffies timeout to
   hrtimers.

There is one patch targeting a hwmon dt-bindings file:
Documentation/devicetree/bindings/hwmon/lm75.yaml. The rest is touching
the I2C bus driver, its bindings and platform devicetrees.

About dependencies:
 - The series is based upon v6.8-rc6.
 - For testing on EyeQ5 hardware and devicetree patches, we need the
   base platform series from Grégory [0] and its dependency [1]. Both
   in mips-next [2].
 - Devicetree commits require the EyeQ5 syscon series [3] that provides
   the reset controller node.
 - The LM75 dt-bindings patch depends on the common schema
   hwmon-common.yaml series from Krzysztof [4]. Found in hwmon-next [5].

Have a nice day,
Théo Lebrun

[0]: https://lore.kernel.org/lkml/20240216174227.409400-1-gregory.clement@bootlin.com/
[1]: https://lore.kernel.org/linux-mips/20240209-regname-v1-0-2125efa016ef@flygoat.com/
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/mips/linux.git/log/
[3]: https://lore.kernel.org/lkml/20240227-mbly-clk-v8-0-c57fbda7664a@bootlin.com/
[4]: https://lore.kernel.org/lkml/20240224-dt-bindings-hwmon-common-v2-0-b446eecf5480@linaro.org/
[5]: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git/log/?h=hwmon-next

To: Linus Walleij <linus.walleij@linaro.org>
To: Andi Shyti <andi.shyti@kernel.org>
To: Rob Herring <robh+dt@kernel.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: <linux-arm-kernel@lists.infradead.org>
Cc: <linux-i2c@vger.kernel.org>
Cc: <devicetree@vger.kernel.org>
Cc: <linux-kernel@vger.kernel.org>
Cc: <linux-mips@vger.kernel.org>
Cc: Gregory Clement <gregory.clement@bootlin.com>
Cc: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Tawfik Bayouk <tawfik.bayouk@mobileye.com>
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Changes in v2:
- dt-bindings: i2c: st,nomadic-i2c:
  - Drop timeout-usecs property, rely on generic i2c-transfer-timeout-us.
  - Use phandle to syscon with cell args; remove mobileye,id prop; move
    mobileye,olb from if-statement to top-level.
- dt-bindings: hwmon: lm75:
  - Inherit from hwmon-common.yaml rather than declare generic label property.
- i2c: nomadik: (ie driver code)
  - Parse i2c-transfer-timeout-us rather than custom timeout-usecs property.
  - Introduce readb/writeb helpers with fallback to readl/writel.
  - Avoid readb() on Mobileye.
  - Use mobileye,olb cell args to get controller index rather than mobileye,id.
  - Take 5 Reviewed-by Linus Walleij.
- MIPS: mobileye: (ie devicetrees)
  - Use mobileye,olb with cell args rather than mobileye,id.
  - Squash reset commit.
  - Add i2c-transfer-timeout-us value of 10ms to all controllers.
  - Rename LM75 instance from tmp112@48 to temperature-sensor@48.
- Link to v1: https://lore.kernel.org/r/20240215-mbly-i2c-v1-0-19a336e91dca@bootlin.com

---
Théo Lebrun (11):
      dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example
      dt-bindings: hwmon: lm75: use common hwmon schema
      i2c: nomadik: rename private struct pointers from dev to priv
      i2c: nomadik: simplify IRQ masking logic
      i2c: nomadik: use bitops helpers
      i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer
      i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout
      i2c: nomadik: fetch i2c-transfer-timeout-us property from devicetree
      i2c: nomadik: support Mobileye EyeQ5 I2C controller
      MIPS: mobileye: eyeq5: add 5 I2C controller nodes
      MIPS: mobileye: eyeq5: add evaluation board I2C temp sensor

 Documentation/devicetree/bindings/hwmon/lm75.yaml  |   3 +-
 .../devicetree/bindings/i2c/st,nomadik-i2c.yaml    |  48 +-
 arch/mips/boot/dts/mobileye/eyeq5-epm5.dts         |   8 +
 arch/mips/boot/dts/mobileye/eyeq5.dtsi             |  75 +++
 drivers/i2c/busses/i2c-nomadik.c                   | 720 ++++++++++++---------
 5 files changed, 541 insertions(+), 313 deletions(-)
---
base-commit: a6cc37d1a531e1c99e7989001a0529b443397900
change-id: 20231023-mbly-i2c-7c2fbbb1299f

Best regards,
-- 
Théo Lebrun <theo.lebrun@bootlin.com>

[PATCH v2 01/11] dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example

[Théo Lebrun]

Add EyeQ5 bindings to the existing Nomadik I2C dt-bindings. Add the
EyeQ5-specific property behind a conditional. Add an example for this
compatible.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 .../devicetree/bindings/i2c/st,nomadik-i2c.yaml    | 48 ++++++++++++++++++++--
 1 file changed, 44 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
index 16024415a4a7..2d9d5b276762 100644
--- a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
@@ -14,9 +14,6 @@ description: The Nomadik I2C host controller began its life in the ST
 maintainers:
   - Linus Walleij <linus.walleij@linaro.org>
 
-allOf:
-  - $ref: /schemas/i2c/i2c-controller.yaml#
-
 # Need a custom select here or 'arm,primecell' will match on lots of nodes
 select:
   properties:
@@ -24,6 +21,7 @@ select:
       contains:
         enum:
           - st,nomadik-i2c
+          - mobileye,eyeq5-i2c
   required:
     - compatible
 
@@ -39,6 +37,10 @@ properties:
           - const: stericsson,db8500-i2c
           - const: st,nomadik-i2c
           - const: arm,primecell
+      # The variant found on Mobileye EyeQ5
+      - items:
+          - const: mobileye,eyeq5-i2c
+          - const: arm,primecell
 
   reg:
     maxItems: 1
@@ -55,7 +57,7 @@ properties:
       - items:
           - const: mclk
           - const: apb_pclk
-      # Clock name in DB8500
+      # Clock name in DB8500 or EyeQ5
       - items:
           - const: i2cclk
           - const: apb_pclk
@@ -70,6 +72,16 @@ properties:
     minimum: 1
     maximum: 400000
 
+  mobileye,olb:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: Phandle to OLB system controller node.
+          - description: Platform-wide controller ID (integer starting from zero).
+    description:
+      The phandle pointing to OLB system controller node, with the I2C
+      controller index.
+
 required:
   - compatible
   - reg
@@ -79,6 +91,20 @@ required:
 
 unevaluatedProperties: false
 
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: mobileye,eyeq5-i2c
+    then:
+      required:
+        - mobileye,olb
+    else:
+      properties:
+        mobileye,olb: false
+
 examples:
   - |
     #include <dt-bindings/interrupt-controller/irq.h>
@@ -111,5 +137,19 @@ examples:
       clocks = <&i2c0clk>, <&pclki2c0>;
       clock-names = "mclk", "apb_pclk";
     };
+  - |
+    #include <dt-bindings/interrupt-controller/mips-gic.h>
+    i2c@300000 {
+      compatible = "mobileye,eyeq5-i2c", "arm,primecell";
+      reg = <0x300000 0x1000>;
+      interrupt-parent = <&gic>;
+      interrupts = <GIC_SHARED 1 IRQ_TYPE_LEVEL_HIGH>;
+      clock-frequency = <400000>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+      clocks = <&i2c_ser_clk>, <&i2c_clk>;
+      clock-names = "i2cclk", "apb_pclk";
+      mobileye,olb = <&olb 0>;
+    };
 
 ...

-- 
2.44.0

Re: [PATCH v2 01/11] dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example

[Rob Herring]

On Thu, 29 Feb 2024 19:10:49 +0100, Théo Lebrun wrote:
> Add EyeQ5 bindings to the existing Nomadik I2C dt-bindings. Add the
> EyeQ5-specific property behind a conditional. Add an example for this
> compatible.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  .../devicetree/bindings/i2c/st,nomadik-i2c.yaml    | 48 ++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 4 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.example.dtb: i2c@300000: 'mobileye,olb' does not match any of the regexes: '^#.*', '^(at25|bm|devbus|dmacap|dsa|exynos|fsi[ab]|gpio-fan|gpio-key|gpio|gpmc|hdmi|i2c-gpio),.*', '^(keypad|m25p|max8952|max8997|max8998|mpmc),.*', '^(pinctrl-single|#pinctrl-single|PowerPC),.*', '^(pl022|pxa-mmc|rcar_sound|rotary-encoder|s5m8767|sdhci),.*', '^(simple-audio-card|st-plgpio|st-spics|ts),.*', '^100ask,.*', '^70mai,.*', '^8dev,.*', '^GEFanuc,.*', '^ORCL,.*', '^SUNW,.*', '^[a-zA-Z0-9#_][a-zA-Z0-9+\\-._@]{0,63}$', '^[a-zA-Z0-9+\\-._]*@[0-9a-zA-Z,]*$', '^abb,.*', '^abilis,.*', '^abracon,.*', '^abt,.*', '^acbel,.*', '^acer,.*', '^acme,.*', '^actions,.*', '^active-semi,.*', '^ad,.*', '^adafruit,.*', '^adapteva,.*', '^adaptrum,.*', '^adh,.*', '^adi,.*', '^adieng,.*', '^advantech,.*', '^aeroflexgaisler,.*', '^aesop,.*', '^airoha,.*', '^al,.*', '^alcatel,.*', '^aldec,.*', '^alfa-network,.*', '^allegro,.*', '^alliedvision,.*', '^allo,.*', '^allwinner,.*', '^alphascale,.*', '^alps,.*', '^alt,.*', '^altr,.*', '^amarula,.*', '^amazon,.*', '^amcc,.*', '^amd,.*', '^amediatech,.*', '^amlogic,.*', '^ampere,.*', '^ampire,.*', '^ams,.*', '^amstaos,.*', '^analogix,.*', '^anbernic,.*', '^andestech,.*', '^anvo,.*', '^aosong,.*', '^apm,.*', '^apple,.*', '^aptina,.*', '^arasan,.*', '^archermind,.*', '^arcom,.*', '^arctic,.*', '^arcx,.*', '^aries,.*', '^arm,.*', '^armadeus,.*', '^arrow,.*', '^artesyn,.*', '^asahi-kasei,.*', '^asc,.*', '^asix,.*', '^aspeed,.*', '^asrock,.*', '^asus,.*', '^atheros,.*', '^atlas,.*', '^atmel,.*', '^auo,.*', '^auvidea,.*', '^avago,.*', '^avia,.*', '^avic,.*', '^avnet,.*', '^awinic,.*', '^axentia,.*', '^axis,.*', '^azoteq,.*', '^azw,.*', '^baikal,.*', '^bananapi,.*', '^beacon,.*', '^beagle,.*', '^belling,.*', '^bhf,.*', '^bigtreetech,.*', '^bitmain,.*', '^blutek,.*', '^boe,.*', '^bosch,.*', '^boundary,.*', '^brcm,.*', '^broadmobi,.*', '^bsh,.*', '^bticino,.*', '^buffalo,.*', '^bur,.*', '^bytedance,.*', '^calamp,.*', '^calaosystems,.*', '^calxeda,.*', '^canaan,.*', '^caninos,.*', '^capella,.*', '^cascoda,.*', '^catalyst,.*', '^cavium,.*', '^cdns,.*', '^cdtech,.*', '^cellwise,.*', '^ceva,.*', '^chargebyte,.*', '^checkpoint,.*', '^chefree,.*', '^chipidea,.*', '^chipone,.*', '^chipspark,.*', '^chongzhou,.*', '^chrontel,.*', '^chrp,.*', '^chunghwa,.*', '^chuwi,.*', '^ciaa,.*', '^cirrus,.*', '^cisco,.*', '^clockwork,.*', '^cloos,.*', '^cloudengines,.*', '^cnm,.*', '^cnxt,.*', '^colorfly,.*', '^compulab,.*', '^congatec,.*', '^coolpi,.*', '^coreriver,.*', '^corpro,.*', '^cortina,.*', '^cosmic,.*', '^crane,.*', '^creative,.*', '^crystalfontz,.*', '^csky,.*', '^csq,.*', '^ctera,.*', '^ctu,.*', '^cubietech,.*', '^cui,.*', '^cypress,.*', '^cyx,.*', '^cznic,.*', '^dallas,.*', '^dataimage,.*', '^davicom,.*', '^dell,.*', '^delta,.*', '^densitron,.*', '^denx,.*', '^devantech,.*', '^dfi,.*', '^dh,.*', '^difrnce,.*', '^digi,.*', '^digilent,.*', '^dimonoff,.*', '^diodes,.*', '^dioo,.*', '^dlc,.*', '^dlg,.*', '^dlink,.*', '^dmo,.*', '^domintech,.*', '^dongwoon,.*', '^dptechnics,.*', '^dragino,.*', '^ds,.*', '^dserve,.*', '^dynaimage,.*', '^ea,.*', '^ebang,.*', '^ebbg,.*', '^ebs-systart,.*', '^ebv,.*', '^eckelmann,.*', '^edgeble,.*', '^edimax,.*', '^edt,.*', '^ees,.*', '^eeti,.*', '^einfochips,.*', '^eink,.*', '^elan,.*', '^element14,.*', '^elgin,.*', '^elida,.*', '^elimo,.*', '^elpida,.*', '^embedfire,.*', '^embest,.*', '^emlid,.*', '^emmicro,.*', '^empire-electronix,.*', '^emtrion,.*', '^enclustra,.*', '^endless,.*', '^ene,.*', '^energymicro,.*', '^engicam,.*', '^engleder,.*', '^epcos,.*', '^epfl,.*', '^epson,.*', '^esp,.*', '^est,.*', '^ettus,.*', '^eukrea,.*', '^everest,.*', '^everspin,.*', '^evervision,.*', '^exar,.*', '^excito,.*', '^exegin,.*', '^ezchip,.*', '^facebook,.*', '^fairphone,.*', '^faraday,.*', '^fascontek,.*', '^fastrax,.*', '^fcs,.*', '^feixin,.*', '^feiyang,.*', '^fii,.*', '^firefly,.*', '^focaltech,.*', '^forlinx,.*', '^freecom,.*', '^frida,.*', '^friendlyarm,.*', '^fsl,.*', '^fujitsu,.*', '^fxtec,.*', '^galaxycore,.*', '^gardena,.*', '^gateway,.*', '^gateworks,.*', '^gcw,.*', '^ge,.*', '^geekbuying,.*', '^gef,.*', '^gemei,.*', '^gemtek,.*', '^genesys,.*', '^geniatech,.*', '^giantec,.*', '^giantplus,.*', '^globalscale,.*', '^globaltop,.*', '^gmt,.*', '^goldelico,.*', '^goodix,.*', '^google,.*', '^goramo,.*', '^gplus,.*', '^grinn,.*', '^grmn,.*', '^gumstix,.*', '^gw,.*', '^hannstar,.*', '^haochuangyi,.*', '^haoyu,.*', '^hardkernel,.*', '^hechuang,.*', '^hideep,.*', '^himax,.*', '^hirschmann,.*', '^hisi,.*', '^hisilicon,.*', '^hit,.*', '^hitex,.*', '^holt,.*', '^holtek,.*', '^honestar,.*', '^honeywell,.*', '^hoperun,.*', '^hp,.*', '^hpe,.*', '^hsg,.*', '^htc,.*', '^huawei,.*', '^hugsun,.*', '^hwacom,.*', '^hxt,.*', '^hycon,.*', '^hydis,.*', '^hynitron,.*', '^hynix,.*', '^hyundai,.*', '^i2se,.*', '^ibm,.*', '^icplus,.*', '^idt,.*', '^ifi,.*', '^ilitek,.*', '^imagis,.*', '^img,.*', '^imi,.*', '^inanbo,.*', '^incircuit,.*', '^indiedroid,.*', '^inet-tek,.*', '^infineon,.*', '^inforce,.*', '^ingenic,.*', '^ingrasys,.*', '^injoinic,.*', '^innocomm,.*', '^innolux,.*', '^inside-secure,.*', '^insignal,.*', '^inspur,.*', '^intel,.*', '^intercontrol,.*', '^invensense,.*', '^inventec,.*', '^inversepath,.*', '^iom,.*', '^irondevice,.*', '^isee,.*', '^isil,.*', '^issi,.*', '^ite,.*', '^itead,.*', '^itian,.*', '^ivo,.*', '^iwave,.*', '^jadard,.*', '^jasonic,.*', '^jdi,.*', '^jedec,.*', '^jesurun,.*', '^jethome,.*', '^jianda,.*', '^joz,.*', '^kam,.*', '^karo,.*', '^keithkoep,.*', '^keymile,.*', '^khadas,.*', '^kiebackpeter,.*', '^kinetic,.*', '^kingdisplay,.*', '^kingnovel,.*', '^kionix,.*', '^kobo,.*', '^kobol,.*', '^koe,.*', '^kontron,.*', '^kosagi,.*', '^kvg,.*', '^kyo,.*', '^lacie,.*', '^laird,.*', '^lamobo,.*', '^lantiq,.*', '^lattice,.*', '^lctech,.*', '^leadtek,.*', '^leez,.*', '^lego,.*', '^lemaker,.*', '^lenovo,.*', '^lg,.*', '^lgphilips,.*', '^libretech,.*', '^licheepi,.*', '^linaro,.*', '^lineartechnology,.*', '^linksprite,.*', '^linksys,.*', '^linutronix,.*', '^linux,.*', '^linx,.*', '^liteon,.*', '^litex,.*', '^lltc,.*', '^logicpd,.*', '^logictechno,.*', '^longcheer,.*', '^lontium,.*', '^loongmasses,.*', '^loongson,.*', '^lsi,.*', '^lunzn,.*', '^lwn,.*', '^lxa,.*', '^m5stack,.*', '^macnica,.*', '^mantix,.*', '^mapleboard,.*', '^marantec,.*', '^marvell,.*', '^maxbotix,.*', '^maxim,.*', '^maxlinear,.*', '^mbvl,.*', '^mcube,.*', '^meas,.*', '^mecer,.*', '^mediatek,.*', '^megachips,.*', '^mele,.*', '^melexis,.*', '^melfas,.*', '^mellanox,.*', '^memsensing,.*', '^memsic,.*', '^menlo,.*', '^mentor,.*', '^meraki,.*', '^merrii,.*', '^methode,.*', '^micrel,.*', '^microchip,.*', '^microcrystal,.*', '^micron,.*', '^microsoft,.*', '^microsys,.*', '^mikroe,.*', '^mikrotik,.*', '^milkv,.*', '^miniand,.*', '^minix,.*', '^miramems,.*', '^mitsubishi,.*', '^mitsumi,.*', '^mixel,.*', '^miyoo,.*', '^mntre,.*', '^modtronix,.*', '^moortec,.*', '^mosaixtech,.*', '^motorcomm,.*', '^motorola,.*', '^moxa,.*', '^mpl,.*', '^mps,.*', '^mqmaker,.*', '^mrvl,.*', '^mscc,.*', '^msi,.*', '^mstar,.*', '^mti,.*', '^multi-inno,.*', '^mundoreader,.*', '^murata,.*', '^mxic,.*', '^mxicy,.*', '^myir,.*', '^national,.*', '^nec,.*', '^neonode,.*', '^netgear,.*', '^netlogic,.*', '^netron-dy,.*', '^netronix,.*', '^netxeon,.*', '^neweast,.*', '^newhaven,.*', '^newvision,.*', '^nexbox,.*', '^nextthing,.*', '^ni,.*', '^nintendo,.*', '^nlt,.*', '^nokia,.*', '^nordic,.*', '^novatek,.*', '^novtech,.*', '^nutsboard,.*', '^nuvoton,.*', '^nvd,.*', '^nvidia,.*', '^nxp,.*', '^oceanic,.*', '^ocs,.*', '^oct,.*', '^okaya,.*', '^oki,.*', '^olimex,.*', '^olpc,.*', '^oneplus,.*', '^onie,.*', '^onion,.*', '^onnn,.*', '^ontat,.*', '^opalkelly,.*', '^openailab,.*', '^opencores,.*', '^openembed,.*', '^openpandora,.*', '^openrisc,.*', '^option,.*', '^oranth,.*', '^orisetech,.*', '^ortustech,.*', '^osddisplays,.*', '^osmc,.*', '^ouya,.*', '^overkiz,.*', '^ovti,.*', '^oxsemi,.*', '^ozzmaker,.*', '^panasonic,.*', '^parade,.*', '^parallax,.*', '^pda,.*', '^pericom,.*', '^pervasive,.*', '^phicomm,.*', '^phytec,.*', '^picochip,.*', '^pine64,.*', '^pineriver,.*', '^pixcir,.*', '^plantower,.*', '^plathome,.*', '^plda,.*', '^plx,.*', '^ply,.*', '^pni,.*', '^pocketbook,.*', '^polaroid,.*', '^polyhex,.*', '^portwell,.*', '^poslab,.*', '^pov,.*', '^powertip,.*', '^powervr,.*', '^powkiddy,.*', '^primux,.*', '^probox2,.*', '^prt,.*', '^pulsedlight,.*', '^purism,.*', '^qca,.*', '^qcom,.*', '^qemu,.*', '^qi,.*', '^qiaodian,.*', '^qihua,.*', '^qishenglong,.*', '^qnap,.*', '^quanta,.*', '^radxa,.*', '^raidsonic,.*', '^ralink,.*', '^ramtron,.*', '^raspberrypi,.*', '^raydium,.*', '^rda,.*', '^realtek,.*', '^remarkable,.*', '^renesas,.*', '^rervision,.*', '^revotics,.*', '^rex,.*', '^richtek,.*', '^ricoh,.*', '^rikomagic,.*', '^riot,.*', '^riscv,.*', '^rockchip,.*', '^rocktech,.*', '^rohm,.*', '^ronbo,.*', '^roofull,.*', '^roseapplepi,.*', '^rve,.*', '^saef,.*', '^samsung,.*', '^samtec,.*', '^sancloud,.*', '^sandisk,.*', '^satoz,.*', '^sbs,.*', '^schindler,.*', '^seagate,.*', '^seeed,.*', '^seirobotics,.*', '^semtech,.*', '^senseair,.*', '^sensirion,.*', '^sensortek,.*', '^sercomm,.*', '^sff,.*', '^sgd,.*', '^sgmicro,.*', '^sgx,.*', '^sharp,.*', '^shift,.*', '^shimafuji,.*', '^shineworld,.*', '^shiratech,.*', '^si-en,.*', '^si-linux,.*', '^siemens,.*', '^sifive,.*', '^sigma,.*', '^sii,.*', '^sil,.*', '^silabs,.*', '^silan,.*', '^silead,.*', '^silergy,.*', '^silex-insight,.*', '^siliconfile,.*', '^siliconmitus,.*', '^silvaco,.*', '^simtek,.*', '^sinlinx,.*', '^sinovoip,.*', '^sinowealth,.*', '^sipeed,.*', '^sirf,.*', '^sis,.*', '^sitronix,.*', '^skov,.*', '^skyworks,.*', '^smartlabs,.*', '^smi,.*', '^smsc,.*', '^snps,.*', '^sochip,.*', '^socionext,.*', '^solidrun,.*', '^solomon,.*', '^sony,.*', '^sophgo,.*', '^sourceparts,.*', '^spansion,.*', '^sparkfun,.*', '^spinalhdl,.*', '^sprd,.*', '^square,.*', '^ssi,.*', '^sst,.*', '^sstar,.*', '^st,.*', '^st-ericsson,.*', '^starfive,.*', '^starry,.*', '^startek,.*', '^starterkit,.*', '^ste,.*', '^stericsson,.*', '^storlink,.*', '^storm,.*', '^storopack,.*', '^summit,.*', '^sunchip,.*', '^sundance,.*', '^sunplus,.*', '^supermicro,.*', '^swir,.*', '^syna,.*', '^synology,.*', '^synopsys,.*', '^tbs,.*', '^tbs-biometrics,.*', '^tcg,.*', '^tcl,.*', '^tcs,.*', '^tdo,.*', '^team-source-display,.*', '^technexion,.*', '^technologic,.*', '^techstar,.*', '^techwell,.*', '^teejet,.*', '^teltonika,.*', '^tempo,.*', '^terasic,.*', '^tesla,.*', '^tfc,.*', '^thead,.*', '^thine,.*', '^thingyjp,.*', '^thundercomm,.*', '^thwc,.*', '^ti,.*', '^tianma,.*', '^tlm,.*', '^tmt,.*', '^topeet,.*', '^topic,.*', '^toppoly,.*', '^topwise,.*', '^toradex,.*', '^toshiba,.*', '^toumaz,.*', '^tpk,.*', '^tplink,.*', '^tpo,.*', '^tq,.*', '^transpeed,.*', '^traverse,.*', '^tronfy,.*', '^tronsmart,.*', '^truly,.*', '^tsd,.*', '^turing,.*', '^tyan,.*', '^u-blox,.*', '^u-boot,.*', '^ubnt,.*', '^ucrobotics,.*', '^udoo,.*', '^ufispace,.*', '^ugoos,.*', '^uniwest,.*', '^upisemi,.*', '^urt,.*', '^usi,.*', '^usr,.*', '^utoo,.*', '^v3,.*', '^vaisala,.*', '^vamrs,.*', '^variscite,.*', '^vdl,.*', '^vertexcom,.*', '^via,.*', '^vialab,.*', '^vicor,.*', '^videostrong,.*', '^virtio,.*', '^virtual,.*', '^vishay,.*', '^visionox,.*', '^vitesse,.*', '^vivante,.*', '^vivax,.*', '^vocore,.*', '^voipac,.*', '^vot,.*', '^vxt,.*', '^wanchanglong,.*', '^wand,.*', '^waveshare,.*', '^wd,.*', '^we,.*', '^welltech,.*', '^wetek,.*', '^wexler,.*', '^whwave,.*', '^wi2wi,.*', '^widora,.*', '^wiligear,.*', '^willsemi,.*', '^winbond,.*', '^wingtech,.*', '^winlink,.*', '^winstar,.*', '^wirelesstag,.*', '^wits,.*', '^wlf,.*', '^wm,.*', '^wobo,.*', '^x-powers,.*', '^xen,.*', '^xes,.*', '^xiaomi,.*', '^xillybus,.*', '^xingbangda,.*', '^xinpeng,.*', '^xiphera,.*', '^xlnx,.*', '^xnano,.*', '^xunlong,.*', '^xylon,.*', '^yadro,.*', '^yamaha,.*', '^yes-optoelectronics,.*', '^yic,.*', '^yiming,.*', '^ylm,.*', '^yna,.*', '^yones-toptech,.*', '^ys,.*', '^ysoft,.*', '^zarlink,.*', '^zealz,.*', '^zeitec,.*', '^zidoo,.*', '^zii,.*', '^zinitix,.*', '^zkmagic,.*', '^zte,.*', '^zyxel,.*', 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/vendor-prefixes.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240229-mbly-i2c-v2-1-b32ed18c098c@bootlin.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 01/11] dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example

[Rob Herring]

On Thu, Feb 29, 2024 at 07:10:49PM +0100, Théo Lebrun wrote:
> Add EyeQ5 bindings to the existing Nomadik I2C dt-bindings. Add the
> EyeQ5-specific property behind a conditional. Add an example for this
> compatible.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  .../devicetree/bindings/i2c/st,nomadik-i2c.yaml    | 48 ++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
> index 16024415a4a7..2d9d5b276762 100644
> --- a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
> @@ -14,9 +14,6 @@ description: The Nomadik I2C host controller began its life in the ST
>  maintainers:
>    - Linus Walleij <linus.walleij@linaro.org>
>  
> -allOf:
> -  - $ref: /schemas/i2c/i2c-controller.yaml#
> -
>  # Need a custom select here or 'arm,primecell' will match on lots of nodes
>  select:
>    properties:
> @@ -24,6 +21,7 @@ select:
>        contains:
>          enum:
>            - st,nomadik-i2c
> +          - mobileye,eyeq5-i2c
>    required:
>      - compatible
>  
> @@ -39,6 +37,10 @@ properties:
>            - const: stericsson,db8500-i2c
>            - const: st,nomadik-i2c
>            - const: arm,primecell
> +      # The variant found on Mobileye EyeQ5

Kind of obvious from the compatible string, but maybe you are keeping 
the existing style...

> +      - items:
> +          - const: mobileye,eyeq5-i2c
> +          - const: arm,primecell
>  
>    reg:
>      maxItems: 1
> @@ -55,7 +57,7 @@ properties:
>        - items:
>            - const: mclk
>            - const: apb_pclk
> -      # Clock name in DB8500
> +      # Clock name in DB8500 or EyeQ5
>        - items:
>            - const: i2cclk
>            - const: apb_pclk
> @@ -70,6 +72,16 @@ properties:
>      minimum: 1
>      maximum: 400000
>  
> +  mobileye,olb:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - items:
> +          - description: Phandle to OLB system controller node.
> +          - description: Platform-wide controller ID (integer starting from zero).

Rather than a made up ID, just store the shift value you ultimately 
need.

These properties are fragile because they break if anything that's not 
defined in DT changes whether that's register offset, bit offset, 
bitfield size or values. Or also if there are additional fields to 
access. 

Rob

Re: [PATCH v2 01/11] dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example

[Théo Lebrun]

Hello,

On Fri Mar 1, 2024 at 4:11 PM CET, Rob Herring wrote:
> On Thu, Feb 29, 2024 at 07:10:49PM +0100, Théo Lebrun wrote:
> > Add EyeQ5 bindings to the existing Nomadik I2C dt-bindings. Add the
> > EyeQ5-specific property behind a conditional. Add an example for this
> > compatible.
> > 
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---
> >  .../devicetree/bindings/i2c/st,nomadik-i2c.yaml    | 48 ++++++++++++++++++++--
> >  1 file changed, 44 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
> > index 16024415a4a7..2d9d5b276762 100644
> > --- a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
> > @@ -14,9 +14,6 @@ description: The Nomadik I2C host controller began its life in the ST
> >  maintainers:
> >    - Linus Walleij <linus.walleij@linaro.org>
> >  
> > -allOf:
> > -  - $ref: /schemas/i2c/i2c-controller.yaml#
> > -
> >  # Need a custom select here or 'arm,primecell' will match on lots of nodes
> >  select:
> >    properties:
> > @@ -24,6 +21,7 @@ select:
> >        contains:
> >          enum:
> >            - st,nomadik-i2c
> > +          - mobileye,eyeq5-i2c
> >    required:
> >      - compatible
> >  
> > @@ -39,6 +37,10 @@ properties:
> >            - const: stericsson,db8500-i2c
> >            - const: st,nomadik-i2c
> >            - const: arm,primecell
> > +      # The variant found on Mobileye EyeQ5
>
> Kind of obvious from the compatible string, but maybe you are keeping 
> the existing style...

I indeed kept the existing style.
Ping me if you want this removed!

> > +      - items:
> > +          - const: mobileye,eyeq5-i2c
> > +          - const: arm,primecell
> >  
> >    reg:
> >      maxItems: 1
> > @@ -55,7 +57,7 @@ properties:
> >        - items:
> >            - const: mclk
> >            - const: apb_pclk
> > -      # Clock name in DB8500
> > +      # Clock name in DB8500 or EyeQ5
> >        - items:
> >            - const: i2cclk
> >            - const: apb_pclk
> > @@ -70,6 +72,16 @@ properties:
> >      minimum: 1
> >      maximum: 400000
> >  
> > +  mobileye,olb:
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    items:
> > +      - items:
> > +          - description: Phandle to OLB system controller node.
> > +          - description: Platform-wide controller ID (integer starting from zero).
>
> Rather than a made up ID, just store the shift value you ultimately 
> need.

Issue with storing the shift value is that you also need to know what
value to put in that field. It's an enum mapping the I2C speed which
isn't found in DT.

> These properties are fragile because they break if anything that's not 
> defined in DT changes whether that's register offset, bit offset, 
> bitfield size or values. Or also if there are additional fields to 
> access.

My take is that it is better to have it all either in DT or in driver.
Having a mix of both is a mess when debugging. If something breaks it
is a driver bug; such bugs get fixed in driver code. That way DT
doesn't know about it and doesn't have to be changed.

Putting shifts in DT is an abstraction that ends up being unhelpful. You
split hardware knowledge half in DT (register offset and/or mask), half
in driver (value to put in the field). We'd rather have it all in
driver code.

Next hardware revision will be more complex with potentially fields
split across registers. A shift value wouldn't cut it. A new
compatible + made up ID allows accomodating for that. That way we have
homogeneity across compatibles.

Have a nice day,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v2 02/11] dt-bindings: hwmon: lm75: use common hwmon schema

[Théo Lebrun]

Reference common hwmon schema which has the generic "label" property,
parsed by Linux hwmon subsystem.

To: Jean Delvare <jdelvare@suse.com>
To: Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 Documentation/devicetree/bindings/hwmon/lm75.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/hwmon/lm75.yaml b/Documentation/devicetree/bindings/hwmon/lm75.yaml
index ed269e428a3d..29bd7460cc26 100644
--- a/Documentation/devicetree/bindings/hwmon/lm75.yaml
+++ b/Documentation/devicetree/bindings/hwmon/lm75.yaml
@@ -57,6 +57,7 @@ required:
   - reg
 
 allOf:
+  - $ref: hwmon-common.yaml#
   - if:
       not:
         properties:
@@ -71,7 +72,7 @@ allOf:
       properties:
         interrupts: false
 
-additionalProperties: false
+unevaluatedProperties: false
 
 examples:
   - |

-- 
2.44.0

Re: [PATCH v2 02/11] dt-bindings: hwmon: lm75: use common hwmon schema

[Rob Herring]

On Thu, 29 Feb 2024 19:10:50 +0100, Théo Lebrun wrote:
> Reference common hwmon schema which has the generic "label" property,
> parsed by Linux hwmon subsystem.
> 
> To: Jean Delvare <jdelvare@suse.com>
> To: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-hwmon@vger.kernel.org
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  Documentation/devicetree/bindings/hwmon/lm75.yaml | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/lm75.yaml:
Error in referenced schema matching $id: http://devicetree.org/schemas/hwmon/hwmon-common.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/lm75.example.dtb: sensor@48: False schema does not allow {'compatible': ['st,stlm75'], 'reg': [[72]], 'vs-supply': [[4294967295]], '$nodename': ['sensor@48']}
	from schema $id: http://devicetree.org/schemas/hwmon/lm75.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/lm75.example.dtb: temperature-sensor@48: False schema does not allow {'compatible': ['ams,as6200'], 'reg': [[72]], 'vs-supply': [[4294967295]], 'interrupts': [[17, 3]], '$nodename': ['temperature-sensor@48']}
	from schema $id: http://devicetree.org/schemas/hwmon/lm75.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240229-mbly-i2c-v2-2-b32ed18c098c@bootlin.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 02/11] dt-bindings: hwmon: lm75: use common hwmon schema

[Krzysztof Kozlowski]

On 29/02/2024 19:10, Théo Lebrun wrote:
> Reference common hwmon schema which has the generic "label" property,
> parsed by Linux hwmon subsystem.
> 

Please do not mix independent patchsets. You create unneeded
dependencies blocking this patch. This patch depends on hwmon work, so
it cannot go through different tree.

If you insist to combine independent patches, then at least clearly
express merging strategy or dependency in patch changelog --- .


> To: Jean Delvare <jdelvare@suse.com>
> To: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-hwmon@vger.kernel.org
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 02/11] dt-bindings: hwmon: lm75: use common hwmon schema

[Guenter Roeck]

On 2/29/24 22:37, Krzysztof Kozlowski wrote:
> On 29/02/2024 19:10, Théo Lebrun wrote:
>> Reference common hwmon schema which has the generic "label" property,
>> parsed by Linux hwmon subsystem.
>>
> 
> Please do not mix independent patchsets. You create unneeded
> dependencies blocking this patch. This patch depends on hwmon work, so
> it cannot go through different tree.
> 
> If you insist to combine independent patches, then at least clearly
> express merging strategy or dependency in patch changelog --- .
> 

For my part I have to say that I don't know what to do with it.
Rob's robot reported errors, so I won't apply it, and I don't
feel comfortable giving it an ack either because of those errors.

Guenter

> 
>> To: Jean Delvare <jdelvare@suse.com>
>> To: Guenter Roeck <linux@roeck-us.net>
>> Cc: linux-hwmon@vger.kernel.org
>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>> ---
> 
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 02/11] dt-bindings: hwmon: lm75: use common hwmon schema

[Théo Lebrun]

Hello,

On Fri Mar 1, 2024 at 7:53 AM CET, Guenter Roeck wrote:
> On 2/29/24 22:37, Krzysztof Kozlowski wrote:
> > On 29/02/2024 19:10, Théo Lebrun wrote:
> >> Reference common hwmon schema which has the generic "label" property,
> >> parsed by Linux hwmon subsystem.
> >>
> > 
> > Please do not mix independent patchsets. You create unneeded
> > dependencies blocking this patch. This patch depends on hwmon work, so
> > it cannot go through different tree.

I had to pick between this or dtbs_check failing on my DTS that uses a
label on temperature-sensor@48.

> > If you insist to combine independent patches, then at least clearly
> > express merging strategy or dependency in patch changelog --- .

I do not know how such indirect conflicts are usually resolved. Hwmon
can take it but MIPS might want to also take it to have valid DTS.

Any advice?

> For my part I have to say that I don't know what to do with it.
> Rob's robot reported errors, so I won't apply it, and I don't
> feel comfortable giving it an ack either because of those errors.

Can reproduce the error when patch "dt-bindings: hwmon: add common
properties" is not applied. Cannot reproduce when patch is applied.
Commit d590900b62f0 on hwmon-next. Cannot reproduce with hwmon-next as
parent.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 02/11] dt-bindings: hwmon: lm75: use common hwmon schema

[Krzysztof Kozlowski]

On 01/03/2024 10:41, Théo Lebrun wrote:
> Hello,
> 
> On Fri Mar 1, 2024 at 7:53 AM CET, Guenter Roeck wrote:
>> On 2/29/24 22:37, Krzysztof Kozlowski wrote:
>>> On 29/02/2024 19:10, Théo Lebrun wrote:
>>>> Reference common hwmon schema which has the generic "label" property,
>>>> parsed by Linux hwmon subsystem.
>>>>
>>>
>>> Please do not mix independent patchsets. You create unneeded
>>> dependencies blocking this patch. This patch depends on hwmon work, so
>>> it cannot go through different tree.
> 
> I had to pick between this or dtbs_check failing on my DTS that uses a
> label on temperature-sensor@48.

I don't see how is that relevant. You can organize your branches as you
wish, e.g. base one b4 branch on another and you will not have any warnings.

> 
>>> If you insist to combine independent patches, then at least clearly
>>> express merging strategy or dependency in patch changelog --- .
> 
> I do not know how such indirect conflicts are usually resolved. Hwmon
> can take it but MIPS might want to also take it to have valid DTS.
> 
> Any advice?

I don't see any conflict.


> 
>> For my part I have to say that I don't know what to do with it.
>> Rob's robot reported errors, so I won't apply it, and I don't
>> feel comfortable giving it an ack either because of those errors.
> 
> Can reproduce the error when patch "dt-bindings: hwmon: add common
> properties" is not applied. Cannot reproduce when patch is applied.
> Commit d590900b62f0 on hwmon-next. Cannot reproduce with hwmon-next as
> parent.

Yeah, but we see the error reported and it means something is missing.

Best regards,
Krzysztof

Re: [PATCH v2 02/11] dt-bindings: hwmon: lm75: use common hwmon schema

[Théo Lebrun]

Hello,

On Fri Mar 1, 2024 at 11:13 AM CET, Krzysztof Kozlowski wrote:
> On 01/03/2024 10:41, Théo Lebrun wrote:
> > Hello,
> > 
> > On Fri Mar 1, 2024 at 7:53 AM CET, Guenter Roeck wrote:
> >> On 2/29/24 22:37, Krzysztof Kozlowski wrote:
> >>> On 29/02/2024 19:10, Théo Lebrun wrote:
> >>>> Reference common hwmon schema which has the generic "label" property,
> >>>> parsed by Linux hwmon subsystem.
> >>>>
> >>>
> >>> Please do not mix independent patchsets. You create unneeded
> >>> dependencies blocking this patch. This patch depends on hwmon work, so
> >>> it cannot go through different tree.
> > 
> > I had to pick between this or dtbs_check failing on my DTS that uses a
> > label on temperature-sensor@48.
>
> I don't see how is that relevant. You can organize your branches as you
> wish, e.g. base one b4 branch on another and you will not have any warnings.

That is what I do, I however do not want mips-next to have errors when
running dtbs_check. Having dtbs_check return errors is not an issue?

> >>> If you insist to combine independent patches, then at least clearly
> >>> express merging strategy or dependency in patch changelog --- .
> > 
> > I do not know how such indirect conflicts are usually resolved. Hwmon
> > can take it but MIPS might want to also take it to have valid DTS.
> > 
> > Any advice?
>
> I don't see any conflict.

I shouldn't have called that a conflict, more like a dependency.

> >> For my part I have to say that I don't know what to do with it.
> >> Rob's robot reported errors, so I won't apply it, and I don't
> >> feel comfortable giving it an ack either because of those errors.
> > 
> > Can reproduce the error when patch "dt-bindings: hwmon: add common
> > properties" is not applied. Cannot reproduce when patch is applied.
> > Commit d590900b62f0 on hwmon-next. Cannot reproduce with hwmon-next as
> > parent.
>
> Yeah, but we see the error reported and it means something is missing.

Yes, this series depends on "dt-bindings: hwmon: add common properties"
which the bot doesn't know it needs to apply.

Should I submit this patch independently and have my DTS be broken wrt
dtbs_check?

Have a nice day,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 02/11] dt-bindings: hwmon: lm75: use common hwmon schema

[Krzysztof Kozlowski]

On 01/03/2024 11:44, Théo Lebrun wrote:
> Hello,
> 
> On Fri Mar 1, 2024 at 11:13 AM CET, Krzysztof Kozlowski wrote:
>> On 01/03/2024 10:41, Théo Lebrun wrote:
>>> Hello,
>>>
>>> On Fri Mar 1, 2024 at 7:53 AM CET, Guenter Roeck wrote:
>>>> On 2/29/24 22:37, Krzysztof Kozlowski wrote:
>>>>> On 29/02/2024 19:10, Théo Lebrun wrote:
>>>>>> Reference common hwmon schema which has the generic "label" property,
>>>>>> parsed by Linux hwmon subsystem.
>>>>>>
>>>>>
>>>>> Please do not mix independent patchsets. You create unneeded
>>>>> dependencies blocking this patch. This patch depends on hwmon work, so
>>>>> it cannot go through different tree.
>>>
>>> I had to pick between this or dtbs_check failing on my DTS that uses a
>>> label on temperature-sensor@48.
>>
>> I don't see how is that relevant. You can organize your branches as you
>> wish, e.g. base one b4 branch on another and you will not have any warnings.
> 
> That is what I do, I however do not want mips-next to have errors when
> running dtbs_check. Having dtbs_check return errors is not an issue?

You should ask your maintainer, but I don't understand how this is
achievable anyway. Subsystem bindings *should not* go via MIPS-next, so
how are you going to solve this?

And why MIPS shall be different than all other ARM/RISC-V SoCs?

Best regards,
Krzysztof

Re: [PATCH v2 02/11] dt-bindings: hwmon: lm75: use common hwmon schema

[Théo Lebrun]

Hello,

On Fri Mar 1, 2024 at 12:35 PM CET, Krzysztof Kozlowski wrote:
> On 01/03/2024 11:44, Théo Lebrun wrote:
> > On Fri Mar 1, 2024 at 11:13 AM CET, Krzysztof Kozlowski wrote:
> >> On 01/03/2024 10:41, Théo Lebrun wrote:
> >>> On Fri Mar 1, 2024 at 7:53 AM CET, Guenter Roeck wrote:
> >>>> On 2/29/24 22:37, Krzysztof Kozlowski wrote:
> >>>>> On 29/02/2024 19:10, Théo Lebrun wrote:
> >>>>>> Reference common hwmon schema which has the generic "label" property,
> >>>>>> parsed by Linux hwmon subsystem.
> >>>>>
> >>>>> Please do not mix independent patchsets. You create unneeded
> >>>>> dependencies blocking this patch. This patch depends on hwmon work, so
> >>>>> it cannot go through different tree.
> >>>
> >>> I had to pick between this or dtbs_check failing on my DTS that uses a
> >>> label on temperature-sensor@48.
> >>
> >> I don't see how is that relevant. You can organize your branches as you
> >> wish, e.g. base one b4 branch on another and you will not have any warnings.
> > 
> > That is what I do, I however do not want mips-next to have errors when
> > running dtbs_check. Having dtbs_check return errors is not an issue?
>
> You should ask your maintainer, but I don't understand how this is
> achievable anyway. Subsystem bindings *should not* go via MIPS-next, so
> how are you going to solve this?

I thought it'd go in hwmon-next and be picked up by mips-next as well.
It's clear now that the right approach is to send the lm75.yaml patch
alone.

I'll wait some more before sending a new revision that drops this
lm75.yaml patch.

Have a nice day,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 02/11] dt-bindings: hwmon: lm75: use common hwmon schema

[Krzysztof Kozlowski]

On 01/03/2024 15:09, Théo Lebrun wrote:
>>>> I don't see how is that relevant. You can organize your branches as you
>>>> wish, e.g. base one b4 branch on another and you will not have any warnings.
>>>
>>> That is what I do, I however do not want mips-next to have errors when
>>> running dtbs_check. Having dtbs_check return errors is not an issue?
>>
>> You should ask your maintainer, but I don't understand how this is
>> achievable anyway. Subsystem bindings *should not* go via MIPS-next, so
>> how are you going to solve this?
> 
> I thought it'd go in hwmon-next and be picked up by mips-next as well.
> It's clear now that the right approach is to send the lm75.yaml patch
> alone.

Then mips-next-dts branch would be based on subsystem branch with driver
changes? That violates the policy of not creating dependencies between
DTS and drivers.

What matters is final or even future release, not intermediate state.

Best regards,
Krzysztof

Re: [PATCH v2 02/11] dt-bindings: hwmon: lm75: use common hwmon schema

[Rob Herring]

On Fri, Mar 01, 2024 at 11:44:37AM +0100, Théo Lebrun wrote:
> Hello,
> 
> On Fri Mar 1, 2024 at 11:13 AM CET, Krzysztof Kozlowski wrote:
> > On 01/03/2024 10:41, Théo Lebrun wrote:
> > > Hello,
> > > 
> > > On Fri Mar 1, 2024 at 7:53 AM CET, Guenter Roeck wrote:
> > >> On 2/29/24 22:37, Krzysztof Kozlowski wrote:
> > >>> On 29/02/2024 19:10, Théo Lebrun wrote:
> > >>>> Reference common hwmon schema which has the generic "label" property,
> > >>>> parsed by Linux hwmon subsystem.
> > >>>>
> > >>>
> > >>> Please do not mix independent patchsets. You create unneeded
> > >>> dependencies blocking this patch. This patch depends on hwmon work, so
> > >>> it cannot go through different tree.
> > > 
> > > I had to pick between this or dtbs_check failing on my DTS that uses a
> > > label on temperature-sensor@48.
> >
> > I don't see how is that relevant. You can organize your branches as you
> > wish, e.g. base one b4 branch on another and you will not have any warnings.
> 
> That is what I do, I however do not want mips-next to have errors when
> running dtbs_check. Having dtbs_check return errors is not an issue?

That's a good goal, but difficult to achieve as you can see. Given 
dtbs_check in general has tons of warnings already, we currently don't 
worry about more warnings in specific branches. We just look at mainline 
and linux-next. And for that it's still so many, I'm just looking at 
general trends. It runs daily here[1].

As we get more platforms trying to stay at zero warnings, then we'll 
need to revisit this. I imagine that will mean all schemas have to go in 
1 branch with acks from subsystem maintainers. That's the opposite of 
what we do now. And then .dts branches will have to merge in the schema 
branch as needed. There are some checks (make dt_compatible_check) to 
for drivers vs. the schemas, so we'd have the same issue with 
intermittent warnings. But the drivers should be more decoupled from the 
schemas than the dts files.

Rob

[1] https://gitlab.com/robherring/linux-dt/-/jobs

Re: [PATCH v2 02/11] dt-bindings: hwmon: lm75: use common hwmon schema

[Théo Lebrun]

Hello,

On Fri Mar 1, 2024 at 4:35 PM CET, Rob Herring wrote:
> On Fri, Mar 01, 2024 at 11:44:37AM +0100, Théo Lebrun wrote:
> > Hello,
> > 
> > On Fri Mar 1, 2024 at 11:13 AM CET, Krzysztof Kozlowski wrote:
> > > On 01/03/2024 10:41, Théo Lebrun wrote:
> > > > Hello,
> > > > 
> > > > On Fri Mar 1, 2024 at 7:53 AM CET, Guenter Roeck wrote:
> > > >> On 2/29/24 22:37, Krzysztof Kozlowski wrote:
> > > >>> On 29/02/2024 19:10, Théo Lebrun wrote:
> > > >>>> Reference common hwmon schema which has the generic "label" property,
> > > >>>> parsed by Linux hwmon subsystem.
> > > >>>>
> > > >>>
> > > >>> Please do not mix independent patchsets. You create unneeded
> > > >>> dependencies blocking this patch. This patch depends on hwmon work, so
> > > >>> it cannot go through different tree.
> > > > 
> > > > I had to pick between this or dtbs_check failing on my DTS that uses a
> > > > label on temperature-sensor@48.
> > >
> > > I don't see how is that relevant. You can organize your branches as you
> > > wish, e.g. base one b4 branch on another and you will not have any warnings.
> > 
> > That is what I do, I however do not want mips-next to have errors when
> > running dtbs_check. Having dtbs_check return errors is not an issue?
>
> That's a good goal, but difficult to achieve as you can see. Given 
> dtbs_check in general has tons of warnings already, we currently don't 
> worry about more warnings in specific branches. We just look at mainline 
> and linux-next. And for that it's still so many, I'm just looking at 
> general trends. It runs daily here[1].

Here's my opportunity to ask a question I've had for a while: do you
have a way to filter out dtbs that are known to be bad actors (ie have
many many warnings)? Maybe a list of platforms you talk about below
that aim at zero warnings?

Another way to ask this: what would be a good default DT_SCHEMA_FILES
value? Not filtering leads to way too many errors.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 02/11] dt-bindings: hwmon: lm75: use common hwmon schema

[Rob Herring]

On Thu, Feb 29, 2024 at 10:53:07PM -0800, Guenter Roeck wrote:
> On 2/29/24 22:37, Krzysztof Kozlowski wrote:
> > On 29/02/2024 19:10, Théo Lebrun wrote:
> > > Reference common hwmon schema which has the generic "label" property,
> > > parsed by Linux hwmon subsystem.
> > > 
> > 
> > Please do not mix independent patchsets. You create unneeded
> > dependencies blocking this patch. This patch depends on hwmon work, so
> > it cannot go through different tree.
> > 
> > If you insist to combine independent patches, then at least clearly
> > express merging strategy or dependency in patch changelog --- .
> > 
> 
> For my part I have to say that I don't know what to do with it.
> Rob's robot reported errors, so I won't apply it, and I don't
> feel comfortable giving it an ack either because of those errors.

You can apply it. Those are just related to not finding 
hwmon-common.yaml which you have, and Théo confirmed it works on 
hwmon-next.

Rob

Re: [PATCH v2 02/11] dt-bindings: hwmon: lm75: use common hwmon schema

[Guenter Roeck]

On Thu, Feb 29, 2024 at 07:10:50PM +0100, Théo Lebrun wrote:
> Reference common hwmon schema which has the generic "label" property,
> parsed by Linux hwmon subsystem.
> 
> To: Jean Delvare <jdelvare@suse.com>
> To: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-hwmon@vger.kernel.org
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Applied to hwmon-next.

Thanks,
Guenter

[PATCH v2 03/11] i2c: nomadik: rename private struct pointers from dev to priv

[Théo Lebrun]

Disambiguate the usage of dev as a variable name; it is usually best to
keep it reserved for struct device pointers. Avoid having multiple
names for the same struct pointer (previously: dev, nmk, nmk_i2c).

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 428 +++++++++++++++++++--------------------
 1 file changed, 214 insertions(+), 214 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index b10574d42b7a..cd511c884f99 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -206,12 +206,12 @@ static inline void i2c_clr_bit(void __iomem *reg, u32 mask)
 
 /**
  * flush_i2c_fifo() - This function flushes the I2C FIFO
- * @dev: private data of I2C Driver
+ * @priv: private data of I2C Driver
  *
  * This function flushes the I2C Tx and Rx FIFOs. It returns
  * 0 on successful flushing of FIFO
  */
-static int flush_i2c_fifo(struct nmk_i2c_dev *dev)
+static int flush_i2c_fifo(struct nmk_i2c_dev *priv)
 {
 #define LOOP_ATTEMPTS 10
 	int i;
@@ -224,19 +224,19 @@ static int flush_i2c_fifo(struct nmk_i2c_dev *dev)
 	 * bits, until then no one must access Tx, Rx FIFO and
 	 * should poll on these bits waiting for the completion.
 	 */
-	writel((I2C_CR_FTX | I2C_CR_FRX), dev->virtbase + I2C_CR);
+	writel((I2C_CR_FTX | I2C_CR_FRX), priv->virtbase + I2C_CR);
 
 	for (i = 0; i < LOOP_ATTEMPTS; i++) {
-		timeout = jiffies + dev->adap.timeout;
+		timeout = jiffies + priv->adap.timeout;
 
 		while (!time_after(jiffies, timeout)) {
-			if ((readl(dev->virtbase + I2C_CR) &
+			if ((readl(priv->virtbase + I2C_CR) &
 				(I2C_CR_FTX | I2C_CR_FRX)) == 0)
-					return 0;
+				return 0;
 		}
 	}
 
-	dev_err(&dev->adev->dev,
+	dev_err(&priv->adev->dev,
 		"flushing operation timed out giving up after %d attempts",
 		LOOP_ATTEMPTS);
 
@@ -245,45 +245,45 @@ static int flush_i2c_fifo(struct nmk_i2c_dev *dev)
 
 /**
  * disable_all_interrupts() - Disable all interrupts of this I2c Bus
- * @dev: private data of I2C Driver
+ * @priv: private data of I2C Driver
  */
-static void disable_all_interrupts(struct nmk_i2c_dev *dev)
+static void disable_all_interrupts(struct nmk_i2c_dev *priv)
 {
 	u32 mask = IRQ_MASK(0);
-	writel(mask, dev->virtbase + I2C_IMSCR);
+	writel(mask, priv->virtbase + I2C_IMSCR);
 }
 
 /**
  * clear_all_interrupts() - Clear all interrupts of I2C Controller
- * @dev: private data of I2C Driver
+ * @priv: private data of I2C Driver
  */
-static void clear_all_interrupts(struct nmk_i2c_dev *dev)
+static void clear_all_interrupts(struct nmk_i2c_dev *priv)
 {
 	u32 mask;
 	mask = IRQ_MASK(I2C_CLEAR_ALL_INTS);
-	writel(mask, dev->virtbase + I2C_ICR);
+	writel(mask, priv->virtbase + I2C_ICR);
 }
 
 /**
  * init_hw() - initialize the I2C hardware
- * @dev: private data of I2C Driver
+ * @priv: private data of I2C Driver
  */
-static int init_hw(struct nmk_i2c_dev *dev)
+static int init_hw(struct nmk_i2c_dev *priv)
 {
 	int stat;
 
-	stat = flush_i2c_fifo(dev);
+	stat = flush_i2c_fifo(priv);
 	if (stat)
 		goto exit;
 
 	/* disable the controller */
-	i2c_clr_bit(dev->virtbase + I2C_CR, I2C_CR_PE);
+	i2c_clr_bit(priv->virtbase + I2C_CR, I2C_CR_PE);
 
-	disable_all_interrupts(dev);
+	disable_all_interrupts(priv);
 
-	clear_all_interrupts(dev);
+	clear_all_interrupts(priv);
 
-	dev->cli.operation = I2C_NO_OPERATION;
+	priv->cli.operation = I2C_NO_OPERATION;
 
 exit:
 	return stat;
@@ -294,15 +294,15 @@ static int init_hw(struct nmk_i2c_dev *dev)
 
 /**
  * load_i2c_mcr_reg() - load the MCR register
- * @dev: private data of controller
+ * @priv: private data of controller
  * @flags: message flags
  */
-static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *dev, u16 flags)
+static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *priv, u16 flags)
 {
 	u32 mcr = 0;
 	unsigned short slave_adr_3msb_bits;
 
-	mcr |= GEN_MASK(dev->cli.slave_adr, I2C_MCR_A7, 1);
+	mcr |= GEN_MASK(priv->cli.slave_adr, I2C_MCR_A7, 1);
 
 	if (unlikely(flags & I2C_M_TEN)) {
 		/* 10-bit address transaction */
@@ -313,7 +313,7 @@ static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *dev, u16 flags)
 		 * the extension (MSB bits) of the 7 bit address loaded
 		 * in A7
 		 */
-		slave_adr_3msb_bits = (dev->cli.slave_adr >> 7) & 0x7;
+		slave_adr_3msb_bits = (priv->cli.slave_adr >> 7) & 0x7;
 
 		mcr |= GEN_MASK(slave_adr_3msb_bits, I2C_MCR_EA10, 8);
 	} else {
@@ -325,40 +325,40 @@ static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *dev, u16 flags)
 	mcr |= GEN_MASK(0, I2C_MCR_SB, 11);
 
 	/* check the operation, master read/write? */
-	if (dev->cli.operation == I2C_WRITE)
+	if (priv->cli.operation == I2C_WRITE)
 		mcr |= GEN_MASK(I2C_WRITE, I2C_MCR_OP, 0);
 	else
 		mcr |= GEN_MASK(I2C_READ, I2C_MCR_OP, 0);
 
 	/* stop or repeated start? */
-	if (dev->stop)
+	if (priv->stop)
 		mcr |= GEN_MASK(1, I2C_MCR_STOP, 14);
 	else
 		mcr &= ~(GEN_MASK(1, I2C_MCR_STOP, 14));
 
-	mcr |= GEN_MASK(dev->cli.count, I2C_MCR_LENGTH, 15);
+	mcr |= GEN_MASK(priv->cli.count, I2C_MCR_LENGTH, 15);
 
 	return mcr;
 }
 
 /**
  * setup_i2c_controller() - setup the controller
- * @dev: private data of controller
+ * @priv: private data of controller
  */
-static void setup_i2c_controller(struct nmk_i2c_dev *dev)
+static void setup_i2c_controller(struct nmk_i2c_dev *priv)
 {
 	u32 brcr1, brcr2;
 	u32 i2c_clk, div;
 	u32 ns;
 	u16 slsu;
 
-	writel(0x0, dev->virtbase + I2C_CR);
-	writel(0x0, dev->virtbase + I2C_HSMCR);
-	writel(0x0, dev->virtbase + I2C_TFTR);
-	writel(0x0, dev->virtbase + I2C_RFTR);
-	writel(0x0, dev->virtbase + I2C_DMAR);
+	writel(0x0, priv->virtbase + I2C_CR);
+	writel(0x0, priv->virtbase + I2C_HSMCR);
+	writel(0x0, priv->virtbase + I2C_TFTR);
+	writel(0x0, priv->virtbase + I2C_RFTR);
+	writel(0x0, priv->virtbase + I2C_DMAR);
 
-	i2c_clk = clk_get_rate(dev->clk);
+	i2c_clk = clk_get_rate(priv->clk);
 
 	/*
 	 * set the slsu:
@@ -373,7 +373,7 @@ static void setup_i2c_controller(struct nmk_i2c_dev *dev)
 	 * slsu = cycles / (1000000000 / f) + 1
 	 */
 	ns = DIV_ROUND_UP_ULL(1000000000ULL, i2c_clk);
-	switch (dev->sm) {
+	switch (priv->sm) {
 	case I2C_FREQ_MODE_FAST:
 	case I2C_FREQ_MODE_FAST_PLUS:
 		slsu = DIV_ROUND_UP(100, ns); /* Fast */
@@ -388,15 +388,15 @@ static void setup_i2c_controller(struct nmk_i2c_dev *dev)
 	}
 	slsu += 1;
 
-	dev_dbg(&dev->adev->dev, "calculated SLSU = %04x\n", slsu);
-	writel(slsu << 16, dev->virtbase + I2C_SCR);
+	dev_dbg(&priv->adev->dev, "calculated SLSU = %04x\n", slsu);
+	writel(slsu << 16, priv->virtbase + I2C_SCR);
 
 	/*
 	 * The spec says, in case of std. mode the divider is
 	 * 2 whereas it is 3 for fast and fastplus mode of
 	 * operation. TODO - high speed support.
 	 */
-	div = (dev->clk_freq > I2C_MAX_STANDARD_MODE_FREQ) ? 3 : 2;
+	div = (priv->clk_freq > I2C_MAX_STANDARD_MODE_FREQ) ? 3 : 2;
 
 	/*
 	 * generate the mask for baud rate counters. The controller
@@ -406,10 +406,10 @@ static void setup_i2c_controller(struct nmk_i2c_dev *dev)
 	 * so set brcr1 to 0.
 	 */
 	brcr1 = 0 << 16;
-	brcr2 = (i2c_clk/(dev->clk_freq * div)) & 0xffff;
+	brcr2 = (i2c_clk / (priv->clk_freq * div)) & 0xffff;
 
 	/* set the baud rate counter register */
-	writel((brcr1 | brcr2), dev->virtbase + I2C_BRCR);
+	writel((brcr1 | brcr2), priv->virtbase + I2C_BRCR);
 
 	/*
 	 * set the speed mode. Currently we support
@@ -417,125 +417,124 @@ static void setup_i2c_controller(struct nmk_i2c_dev *dev)
 	 * TODO - support for fast mode plus (up to 1Mb/s)
 	 * and high speed (up to 3.4 Mb/s)
 	 */
-	if (dev->sm > I2C_FREQ_MODE_FAST) {
-		dev_err(&dev->adev->dev,
+	if (priv->sm > I2C_FREQ_MODE_FAST) {
+		dev_err(&priv->adev->dev,
 			"do not support this mode defaulting to std. mode\n");
 		brcr2 = i2c_clk / (I2C_MAX_STANDARD_MODE_FREQ * 2) & 0xffff;
-		writel((brcr1 | brcr2), dev->virtbase + I2C_BRCR);
+		writel((brcr1 | brcr2), priv->virtbase + I2C_BRCR);
 		writel(I2C_FREQ_MODE_STANDARD << 4,
-				dev->virtbase + I2C_CR);
+				priv->virtbase + I2C_CR);
 	}
-	writel(dev->sm << 4, dev->virtbase + I2C_CR);
+	writel(priv->sm << 4, priv->virtbase + I2C_CR);
 
 	/* set the Tx and Rx FIFO threshold */
-	writel(dev->tft, dev->virtbase + I2C_TFTR);
-	writel(dev->rft, dev->virtbase + I2C_RFTR);
+	writel(priv->tft, priv->virtbase + I2C_TFTR);
+	writel(priv->rft, priv->virtbase + I2C_RFTR);
 }
 
 /**
  * read_i2c() - Read from I2C client device
- * @dev: private data of I2C Driver
+ * @priv: private data of I2C Driver
  * @flags: message flags
  *
  * This function reads from i2c client device when controller is in
  * master mode. There is a completion timeout. If there is no transfer
  * before timeout error is returned.
  */
-static int read_i2c(struct nmk_i2c_dev *dev, u16 flags)
+static int read_i2c(struct nmk_i2c_dev *priv, u16 flags)
 {
 	int status = 0;
 	u32 mcr, irq_mask;
 	unsigned long timeout;
 
-	mcr = load_i2c_mcr_reg(dev, flags);
-	writel(mcr, dev->virtbase + I2C_MCR);
+	mcr = load_i2c_mcr_reg(priv, flags);
+	writel(mcr, priv->virtbase + I2C_MCR);
 
 	/* load the current CR value */
-	writel(readl(dev->virtbase + I2C_CR) | DEFAULT_I2C_REG_CR,
-			dev->virtbase + I2C_CR);
+	writel(readl(priv->virtbase + I2C_CR) | DEFAULT_I2C_REG_CR,
+	       priv->virtbase + I2C_CR);
 
 	/* enable the controller */
-	i2c_set_bit(dev->virtbase + I2C_CR, I2C_CR_PE);
+	i2c_set_bit(priv->virtbase + I2C_CR, I2C_CR_PE);
 
-	init_completion(&dev->xfer_complete);
+	init_completion(&priv->xfer_complete);
 
 	/* enable interrupts by setting the mask */
 	irq_mask = (I2C_IT_RXFNF | I2C_IT_RXFF |
 			I2C_IT_MAL | I2C_IT_BERR);
 
-	if (dev->stop || !dev->vendor->has_mtdws)
+	if (priv->stop || !priv->vendor->has_mtdws)
 		irq_mask |= I2C_IT_MTD;
 	else
 		irq_mask |= I2C_IT_MTDWS;
 
 	irq_mask = I2C_CLEAR_ALL_INTS & IRQ_MASK(irq_mask);
 
-	writel(readl(dev->virtbase + I2C_IMSCR) | irq_mask,
-			dev->virtbase + I2C_IMSCR);
+	writel(readl(priv->virtbase + I2C_IMSCR) | irq_mask,
+	       priv->virtbase + I2C_IMSCR);
 
 	timeout = wait_for_completion_timeout(
-		&dev->xfer_complete, dev->adap.timeout);
+		&priv->xfer_complete, priv->adap.timeout);
 
 	if (timeout == 0) {
 		/* Controller timed out */
-		dev_err(&dev->adev->dev, "read from slave 0x%x timed out\n",
-				dev->cli.slave_adr);
+		dev_err(&priv->adev->dev, "read from slave 0x%x timed out\n",
+			priv->cli.slave_adr);
 		status = -ETIMEDOUT;
 	}
 	return status;
 }
 
-static void fill_tx_fifo(struct nmk_i2c_dev *dev, int no_bytes)
+static void fill_tx_fifo(struct nmk_i2c_dev *priv, int no_bytes)
 {
 	int count;
 
 	for (count = (no_bytes - 2);
 			(count > 0) &&
-			(dev->cli.count != 0);
+			(priv->cli.count != 0);
 			count--) {
 		/* write to the Tx FIFO */
-		writeb(*dev->cli.buffer,
-			dev->virtbase + I2C_TFR);
-		dev->cli.buffer++;
-		dev->cli.count--;
-		dev->cli.xfer_bytes++;
+		writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
+		priv->cli.buffer++;
+		priv->cli.count--;
+		priv->cli.xfer_bytes++;
 	}
 
 }
 
 /**
  * write_i2c() - Write data to I2C client.
- * @dev: private data of I2C Driver
+ * @priv: private data of I2C Driver
  * @flags: message flags
  *
  * This function writes data to I2C client
  */
-static int write_i2c(struct nmk_i2c_dev *dev, u16 flags)
+static int write_i2c(struct nmk_i2c_dev *priv, u16 flags)
 {
 	u32 status = 0;
 	u32 mcr, irq_mask;
 	unsigned long timeout;
 
-	mcr = load_i2c_mcr_reg(dev, flags);
+	mcr = load_i2c_mcr_reg(priv, flags);
 
-	writel(mcr, dev->virtbase + I2C_MCR);
+	writel(mcr, priv->virtbase + I2C_MCR);
 
 	/* load the current CR value */
-	writel(readl(dev->virtbase + I2C_CR) | DEFAULT_I2C_REG_CR,
-			dev->virtbase + I2C_CR);
+	writel(readl(priv->virtbase + I2C_CR) | DEFAULT_I2C_REG_CR,
+	       priv->virtbase + I2C_CR);
 
 	/* enable the controller */
-	i2c_set_bit(dev->virtbase + I2C_CR, I2C_CR_PE);
+	i2c_set_bit(priv->virtbase + I2C_CR, I2C_CR_PE);
 
-	init_completion(&dev->xfer_complete);
+	init_completion(&priv->xfer_complete);
 
 	/* enable interrupts by settings the masks */
 	irq_mask = (I2C_IT_TXFOVR | I2C_IT_MAL | I2C_IT_BERR);
 
 	/* Fill the TX FIFO with transmit data */
-	fill_tx_fifo(dev, MAX_I2C_FIFO_THRESHOLD);
+	fill_tx_fifo(priv, MAX_I2C_FIFO_THRESHOLD);
 
-	if (dev->cli.count != 0)
+	if (priv->cli.count != 0)
 		irq_mask |= I2C_IT_TXFNE;
 
 	/*
@@ -543,23 +542,23 @@ static int write_i2c(struct nmk_i2c_dev *dev, u16 flags)
 	 * set the MTDWS bit (Master Transaction Done Without Stop)
 	 * to start repeated start operation
 	 */
-	if (dev->stop || !dev->vendor->has_mtdws)
+	if (priv->stop || !priv->vendor->has_mtdws)
 		irq_mask |= I2C_IT_MTD;
 	else
 		irq_mask |= I2C_IT_MTDWS;
 
 	irq_mask = I2C_CLEAR_ALL_INTS & IRQ_MASK(irq_mask);
 
-	writel(readl(dev->virtbase + I2C_IMSCR) | irq_mask,
-			dev->virtbase + I2C_IMSCR);
+	writel(readl(priv->virtbase + I2C_IMSCR) | irq_mask,
+	       priv->virtbase + I2C_IMSCR);
 
 	timeout = wait_for_completion_timeout(
-		&dev->xfer_complete, dev->adap.timeout);
+		&priv->xfer_complete, priv->adap.timeout);
 
 	if (timeout == 0) {
 		/* Controller timed out */
-		dev_err(&dev->adev->dev, "write to slave 0x%x timed out\n",
-				dev->cli.slave_adr);
+		dev_err(&priv->adev->dev, "write to slave 0x%x timed out\n",
+			priv->cli.slave_adr);
 		status = -ETIMEDOUT;
 	}
 
@@ -568,28 +567,28 @@ static int write_i2c(struct nmk_i2c_dev *dev, u16 flags)
 
 /**
  * nmk_i2c_xfer_one() - transmit a single I2C message
- * @dev: device with a message encoded into it
+ * @priv: device with a message encoded into it
  * @flags: message flags
  */
-static int nmk_i2c_xfer_one(struct nmk_i2c_dev *dev, u16 flags)
+static int nmk_i2c_xfer_one(struct nmk_i2c_dev *priv, u16 flags)
 {
 	int status;
 
 	if (flags & I2C_M_RD) {
 		/* read operation */
-		dev->cli.operation = I2C_READ;
-		status = read_i2c(dev, flags);
+		priv->cli.operation = I2C_READ;
+		status = read_i2c(priv, flags);
 	} else {
 		/* write operation */
-		dev->cli.operation = I2C_WRITE;
-		status = write_i2c(dev, flags);
+		priv->cli.operation = I2C_WRITE;
+		status = write_i2c(priv, flags);
 	}
 
-	if (status || (dev->result)) {
+	if (status || priv->result) {
 		u32 i2c_sr;
 		u32 cause;
 
-		i2c_sr = readl(dev->virtbase + I2C_SR);
+		i2c_sr = readl(priv->virtbase + I2C_SR);
 		/*
 		 * Check if the controller I2C operation status
 		 * is set to ABORT(11b).
@@ -597,15 +596,15 @@ static int nmk_i2c_xfer_one(struct nmk_i2c_dev *dev, u16 flags)
 		if (((i2c_sr >> 2) & 0x3) == 0x3) {
 			/* get the abort cause */
 			cause =	(i2c_sr >> 4) & 0x7;
-			dev_err(&dev->adev->dev, "%s\n",
+			dev_err(&priv->adev->dev, "%s\n",
 				cause >= ARRAY_SIZE(abort_causes) ?
 				"unknown reason" :
 				abort_causes[cause]);
 		}
 
-		(void) init_hw(dev);
+		init_hw(priv);
 
-		status = status ? status : dev->result;
+		status = status ? status : priv->result;
 	}
 
 	return status;
@@ -663,24 +662,24 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 {
 	int status = 0;
 	int i;
-	struct nmk_i2c_dev *dev = i2c_get_adapdata(i2c_adap);
+	struct nmk_i2c_dev *priv = i2c_get_adapdata(i2c_adap);
 	int j;
 
-	pm_runtime_get_sync(&dev->adev->dev);
+	pm_runtime_get_sync(&priv->adev->dev);
 
 	/* Attempt three times to send the message queue */
 	for (j = 0; j < 3; j++) {
 		/* setup the i2c controller */
-		setup_i2c_controller(dev);
+		setup_i2c_controller(priv);
 
 		for (i = 0; i < num_msgs; i++) {
-			dev->cli.slave_adr	= msgs[i].addr;
-			dev->cli.buffer		= msgs[i].buf;
-			dev->cli.count		= msgs[i].len;
-			dev->stop = (i < (num_msgs - 1)) ? 0 : 1;
-			dev->result = 0;
+			priv->cli.slave_adr	= msgs[i].addr;
+			priv->cli.buffer		= msgs[i].buf;
+			priv->cli.count		= msgs[i].len;
+			priv->stop = (i < (num_msgs - 1)) ? 0 : 1;
+			priv->result = 0;
 
-			status = nmk_i2c_xfer_one(dev, msgs[i].flags);
+			status = nmk_i2c_xfer_one(priv, msgs[i].flags);
 			if (status != 0)
 				break;
 		}
@@ -688,7 +687,7 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 			break;
 	}
 
-	pm_runtime_put_sync(&dev->adev->dev);
+	pm_runtime_put_sync(&priv->adev->dev);
 
 	/* return the no. messages processed */
 	if (status)
@@ -699,14 +698,14 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 
 /**
  * disable_interrupts() - disable the interrupts
- * @dev: private data of controller
+ * @priv: private data of controller
  * @irq: interrupt number
  */
-static int disable_interrupts(struct nmk_i2c_dev *dev, u32 irq)
+static int disable_interrupts(struct nmk_i2c_dev *priv, u32 irq)
 {
 	irq = IRQ_MASK(irq);
-	writel(readl(dev->virtbase + I2C_IMSCR) & ~(I2C_CLEAR_ALL_INTS & irq),
-			dev->virtbase + I2C_IMSCR);
+	writel(readl(priv->virtbase + I2C_IMSCR) & ~(I2C_CLEAR_ALL_INTS & irq),
+	       priv->virtbase + I2C_IMSCR);
 	return 0;
 }
 
@@ -723,17 +722,18 @@ static int disable_interrupts(struct nmk_i2c_dev *dev, u32 irq)
  */
 static irqreturn_t i2c_irq_handler(int irq, void *arg)
 {
-	struct nmk_i2c_dev *dev = arg;
+	struct nmk_i2c_dev *priv = arg;
+	struct device *dev = &priv->adev->dev;
 	u32 tft, rft;
 	u32 count;
 	u32 misr, src;
 
 	/* load Tx FIFO and Rx FIFO threshold values */
-	tft = readl(dev->virtbase + I2C_TFTR);
-	rft = readl(dev->virtbase + I2C_RFTR);
+	tft = readl(priv->virtbase + I2C_TFTR);
+	rft = readl(priv->virtbase + I2C_RFTR);
 
 	/* read interrupt status register */
-	misr = readl(dev->virtbase + I2C_MISR);
+	misr = readl(priv->virtbase + I2C_MISR);
 
 	src = __ffs(misr);
 	switch ((1 << src)) {
@@ -741,20 +741,20 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	/* Transmit FIFO nearly empty interrupt */
 	case I2C_IT_TXFNE:
 	{
-		if (dev->cli.operation == I2C_READ) {
+		if (priv->cli.operation == I2C_READ) {
 			/*
 			 * in read operation why do we care for writing?
 			 * so disable the Transmit FIFO interrupt
 			 */
-			disable_interrupts(dev, I2C_IT_TXFNE);
+			disable_interrupts(priv, I2C_IT_TXFNE);
 		} else {
-			fill_tx_fifo(dev, (MAX_I2C_FIFO_THRESHOLD - tft));
+			fill_tx_fifo(priv, (MAX_I2C_FIFO_THRESHOLD - tft));
 			/*
 			 * if done, close the transfer by disabling the
 			 * corresponding TXFNE interrupt
 			 */
-			if (dev->cli.count == 0)
-				disable_interrupts(dev,	I2C_IT_TXFNE);
+			if (priv->cli.count == 0)
+				disable_interrupts(priv,	I2C_IT_TXFNE);
 		}
 	}
 	break;
@@ -768,60 +768,59 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	case I2C_IT_RXFNF:
 		for (count = rft; count > 0; count--) {
 			/* Read the Rx FIFO */
-			*dev->cli.buffer = readb(dev->virtbase + I2C_RFR);
-			dev->cli.buffer++;
+			*priv->cli.buffer = readb(priv->virtbase + I2C_RFR);
+			priv->cli.buffer++;
 		}
-		dev->cli.count -= rft;
-		dev->cli.xfer_bytes += rft;
+		priv->cli.count -= rft;
+		priv->cli.xfer_bytes += rft;
 		break;
 
 	/* Rx FIFO full */
 	case I2C_IT_RXFF:
 		for (count = MAX_I2C_FIFO_THRESHOLD; count > 0; count--) {
-			*dev->cli.buffer = readb(dev->virtbase + I2C_RFR);
-			dev->cli.buffer++;
+			*priv->cli.buffer = readb(priv->virtbase + I2C_RFR);
+			priv->cli.buffer++;
 		}
-		dev->cli.count -= MAX_I2C_FIFO_THRESHOLD;
-		dev->cli.xfer_bytes += MAX_I2C_FIFO_THRESHOLD;
+		priv->cli.count -= MAX_I2C_FIFO_THRESHOLD;
+		priv->cli.xfer_bytes += MAX_I2C_FIFO_THRESHOLD;
 		break;
 
 	/* Master Transaction Done with/without stop */
 	case I2C_IT_MTD:
 	case I2C_IT_MTDWS:
-		if (dev->cli.operation == I2C_READ) {
-			while (!(readl(dev->virtbase + I2C_RISR)
+		if (priv->cli.operation == I2C_READ) {
+			while (!(readl(priv->virtbase + I2C_RISR)
 				 & I2C_IT_RXFE)) {
-				if (dev->cli.count == 0)
+				if (priv->cli.count == 0)
 					break;
-				*dev->cli.buffer =
-					readb(dev->virtbase + I2C_RFR);
-				dev->cli.buffer++;
-				dev->cli.count--;
-				dev->cli.xfer_bytes++;
+				*priv->cli.buffer =
+					readb(priv->virtbase + I2C_RFR);
+				priv->cli.buffer++;
+				priv->cli.count--;
+				priv->cli.xfer_bytes++;
 			}
 		}
 
-		disable_all_interrupts(dev);
-		clear_all_interrupts(dev);
+		disable_all_interrupts(priv);
+		clear_all_interrupts(priv);
 
-		if (dev->cli.count) {
-			dev->result = -EIO;
-			dev_err(&dev->adev->dev,
-				"%lu bytes still remain to be xfered\n",
-				dev->cli.count);
-			(void) init_hw(dev);
+		if (priv->cli.count) {
+			priv->result = -EIO;
+			dev_err(dev, "%lu bytes still remain to be xfered\n",
+				priv->cli.count);
+			init_hw(priv);
 		}
-		complete(&dev->xfer_complete);
+		complete(&priv->xfer_complete);
 
 		break;
 
 	/* Master Arbitration lost interrupt */
 	case I2C_IT_MAL:
-		dev->result = -EIO;
-		(void) init_hw(dev);
+		priv->result = -EIO;
+		init_hw(priv);
 
-		i2c_set_bit(dev->virtbase + I2C_ICR, I2C_IT_MAL);
-		complete(&dev->xfer_complete);
+		i2c_set_bit(priv->virtbase + I2C_ICR, I2C_IT_MAL);
+		complete(&priv->xfer_complete);
 
 		break;
 
@@ -831,13 +830,13 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	 * during the transaction.
 	 */
 	case I2C_IT_BERR:
-		dev->result = -EIO;
+		priv->result = -EIO;
 		/* get the status */
-		if (((readl(dev->virtbase + I2C_SR) >> 2) & 0x3) == I2C_ABORT)
-			(void) init_hw(dev);
+		if (((readl(priv->virtbase + I2C_SR) >> 2) & 0x3) == I2C_ABORT)
+			init_hw(priv);
 
-		i2c_set_bit(dev->virtbase + I2C_ICR, I2C_IT_BERR);
-		complete(&dev->xfer_complete);
+		i2c_set_bit(priv->virtbase + I2C_ICR, I2C_IT_BERR);
+		complete(&priv->xfer_complete);
 
 		break;
 
@@ -847,11 +846,11 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	 * the Tx FIFO is full.
 	 */
 	case I2C_IT_TXFOVR:
-		dev->result = -EIO;
-		(void) init_hw(dev);
+		priv->result = -EIO;
+		init_hw(priv);
 
-		dev_err(&dev->adev->dev, "Tx Fifo Over run\n");
-		complete(&dev->xfer_complete);
+		dev_err(dev, "Tx Fifo Over run\n");
+		complete(&priv->xfer_complete);
 
 		break;
 
@@ -863,10 +862,10 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	case I2C_IT_RFSE:
 	case I2C_IT_WTSR:
 	case I2C_IT_STD:
-		dev_err(&dev->adev->dev, "unhandled Interrupt\n");
+		dev_err(dev, "unhandled Interrupt\n");
 		break;
 	default:
-		dev_err(&dev->adev->dev, "spurious Interrupt..\n");
+		dev_err(dev, "spurious Interrupt..\n");
 		break;
 	}
 
@@ -893,9 +892,9 @@ static int nmk_i2c_resume_early(struct device *dev)
 static int nmk_i2c_runtime_suspend(struct device *dev)
 {
 	struct amba_device *adev = to_amba_device(dev);
-	struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
+	struct nmk_i2c_dev *priv = amba_get_drvdata(adev);
 
-	clk_disable_unprepare(nmk_i2c->clk);
+	clk_disable_unprepare(priv->clk);
 	pinctrl_pm_select_idle_state(dev);
 	return 0;
 }
@@ -903,10 +902,10 @@ static int nmk_i2c_runtime_suspend(struct device *dev)
 static int nmk_i2c_runtime_resume(struct device *dev)
 {
 	struct amba_device *adev = to_amba_device(dev);
-	struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
+	struct nmk_i2c_dev *priv = amba_get_drvdata(adev);
 	int ret;
 
-	ret = clk_prepare_enable(nmk_i2c->clk);
+	ret = clk_prepare_enable(priv->clk);
 	if (ret) {
 		dev_err(dev, "can't prepare_enable clock\n");
 		return ret;
@@ -914,9 +913,9 @@ static int nmk_i2c_runtime_resume(struct device *dev)
 
 	pinctrl_pm_select_default_state(dev);
 
-	ret = init_hw(nmk_i2c);
+	ret = init_hw(priv);
 	if (ret) {
-		clk_disable_unprepare(nmk_i2c->clk);
+		clk_disable_unprepare(priv->clk);
 		pinctrl_pm_select_idle_state(dev);
 	}
 
@@ -939,107 +938,108 @@ static const struct i2c_algorithm nmk_i2c_algo = {
 };
 
 static void nmk_i2c_of_probe(struct device_node *np,
-			     struct nmk_i2c_dev *nmk)
+			     struct nmk_i2c_dev *priv)
 {
 	/* Default to 100 kHz if no frequency is given in the node */
-	if (of_property_read_u32(np, "clock-frequency", &nmk->clk_freq))
-		nmk->clk_freq = I2C_MAX_STANDARD_MODE_FREQ;
+	if (of_property_read_u32(np, "clock-frequency", &priv->clk_freq))
+		priv->clk_freq = I2C_MAX_STANDARD_MODE_FREQ;
 
 	/* This driver only supports 'standard' and 'fast' modes of operation. */
-	if (nmk->clk_freq <= I2C_MAX_STANDARD_MODE_FREQ)
-		nmk->sm = I2C_FREQ_MODE_STANDARD;
+	if (priv->clk_freq <= I2C_MAX_STANDARD_MODE_FREQ)
+		priv->sm = I2C_FREQ_MODE_STANDARD;
 	else
-		nmk->sm = I2C_FREQ_MODE_FAST;
-	nmk->tft = 1; /* Tx FIFO threshold */
-	nmk->rft = 8; /* Rx FIFO threshold */
-	nmk->timeout = 200; /* Slave response timeout(ms) */
+		priv->sm = I2C_FREQ_MODE_FAST;
+	priv->tft = 1; /* Tx FIFO threshold */
+	priv->rft = 8; /* Rx FIFO threshold */
+	priv->timeout = 200; /* Slave response timeout(ms) */
 }
 
 static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 {
 	int ret = 0;
+	struct nmk_i2c_dev *priv;
 	struct device_node *np = adev->dev.of_node;
-	struct nmk_i2c_dev	*dev;
+	struct device *dev = &adev->dev;
 	struct i2c_adapter *adap;
 	struct i2c_vendor_data *vendor = id->data;
 	u32 max_fifo_threshold = (vendor->fifodepth / 2) - 1;
 
-	dev = devm_kzalloc(&adev->dev, sizeof(*dev), GFP_KERNEL);
-	if (!dev)
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
 		return -ENOMEM;
 
-	dev->vendor = vendor;
-	dev->adev = adev;
-	nmk_i2c_of_probe(np, dev);
+	priv->vendor = vendor;
+	priv->adev = adev;
+	nmk_i2c_of_probe(np, priv);
 
-	if (dev->tft > max_fifo_threshold) {
-		dev_warn(&adev->dev, "requested TX FIFO threshold %u, adjusted down to %u\n",
-			 dev->tft, max_fifo_threshold);
-		dev->tft = max_fifo_threshold;
+	if (priv->tft > max_fifo_threshold) {
+		dev_warn(dev, "requested TX FIFO threshold %u, adjusted down to %u\n",
+			 priv->tft, max_fifo_threshold);
+		priv->tft = max_fifo_threshold;
 	}
 
-	if (dev->rft > max_fifo_threshold) {
-		dev_warn(&adev->dev, "requested RX FIFO threshold %u, adjusted down to %u\n",
-			dev->rft, max_fifo_threshold);
-		dev->rft = max_fifo_threshold;
+	if (priv->rft > max_fifo_threshold) {
+		dev_warn(dev, "requested RX FIFO threshold %u, adjusted down to %u\n",
+			 priv->rft, max_fifo_threshold);
+		priv->rft = max_fifo_threshold;
 	}
 
-	amba_set_drvdata(adev, dev);
+	amba_set_drvdata(adev, priv);
 
-	dev->virtbase = devm_ioremap(&adev->dev, adev->res.start,
-				resource_size(&adev->res));
-	if (!dev->virtbase)
+	priv->virtbase = devm_ioremap(dev, adev->res.start,
+				      resource_size(&adev->res));
+	if (!priv->virtbase)
 		return -ENOMEM;
 
-	dev->irq = adev->irq[0];
-	ret = devm_request_irq(&adev->dev, dev->irq, i2c_irq_handler, 0,
-				DRIVER_NAME, dev);
+	priv->irq = adev->irq[0];
+	ret = devm_request_irq(dev, priv->irq, i2c_irq_handler, 0,
+			       DRIVER_NAME, priv);
 	if (ret)
-		return dev_err_probe(&adev->dev, ret,
-				     "cannot claim the irq %d\n", dev->irq);
+		return dev_err_probe(dev, ret,
+				     "cannot claim the irq %d\n", priv->irq);
 
-	dev->clk = devm_clk_get_enabled(&adev->dev, NULL);
-	if (IS_ERR(dev->clk))
-		return dev_err_probe(&adev->dev, PTR_ERR(dev->clk),
+	priv->clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(priv->clk))
+		return dev_err_probe(dev, PTR_ERR(priv->clk),
 				     "could enable i2c clock\n");
 
-	init_hw(dev);
+	init_hw(priv);
 
-	adap = &dev->adap;
+	adap = &priv->adap;
 	adap->dev.of_node = np;
-	adap->dev.parent = &adev->dev;
+	adap->dev.parent = dev;
 	adap->owner = THIS_MODULE;
 	adap->class = I2C_CLASS_DEPRECATED;
 	adap->algo = &nmk_i2c_algo;
-	adap->timeout = msecs_to_jiffies(dev->timeout);
+	adap->timeout = msecs_to_jiffies(priv->timeout);
 	snprintf(adap->name, sizeof(adap->name),
 		 "Nomadik I2C at %pR", &adev->res);
 
-	i2c_set_adapdata(adap, dev);
+	i2c_set_adapdata(adap, priv);
 
-	dev_info(&adev->dev,
+	dev_info(dev,
 		 "initialize %s on virtual base %p\n",
-		 adap->name, dev->virtbase);
+		 adap->name, priv->virtbase);
 
 	ret = i2c_add_adapter(adap);
 	if (ret)
 		return ret;
 
-	pm_runtime_put(&adev->dev);
+	pm_runtime_put(dev);
 
 	return 0;
 }
 
 static void nmk_i2c_remove(struct amba_device *adev)
 {
-	struct nmk_i2c_dev *dev = amba_get_drvdata(adev);
+	struct nmk_i2c_dev *priv = amba_get_drvdata(adev);
 
-	i2c_del_adapter(&dev->adap);
-	flush_i2c_fifo(dev);
-	disable_all_interrupts(dev);
-	clear_all_interrupts(dev);
+	i2c_del_adapter(&priv->adap);
+	flush_i2c_fifo(priv);
+	disable_all_interrupts(priv);
+	clear_all_interrupts(priv);
 	/* disable the controller */
-	i2c_clr_bit(dev->virtbase + I2C_CR, I2C_CR_PE);
+	i2c_clr_bit(priv->virtbase + I2C_CR, I2C_CR_PE);
 }
 
 static struct i2c_vendor_data vendor_stn8815 = {

-- 
2.44.0

Re: [SPAM] [PATCH v2 03/11] i2c: nomadik: rename private struct pointers from dev to priv

[Andi Shyti]

Hi Theo,

On Thu, Feb 29, 2024 at 07:10:51PM +0100, Théo Lebrun wrote:
> Disambiguate the usage of dev as a variable name; it is usually best to
> keep it reserved for struct device pointers. Avoid having multiple
> names for the same struct pointer (previously: dev, nmk, nmk_i2c).

just a nitpick here: this patch does also some small style fixes.
Could you please mention them in the commit log in your v3?

> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

In any case,

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Thanks,
Andi

Re: [PATCH v2 03/11] i2c: nomadik: rename private struct pointers from dev to priv

[Wolfram Sang]

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

On Thu, Feb 29, 2024 at 07:10:51PM +0100, Théo Lebrun wrote:
> Disambiguate the usage of dev as a variable name; it is usually best to
> keep it reserved for struct device pointers. Avoid having multiple
> names for the same struct pointer (previously: dev, nmk, nmk_i2c).
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

I think this improves readability a lot. I didn't really review, but I
do like such changes:

Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


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

[PATCH v2 04/11] i2c: nomadik: simplify IRQ masking logic

[Théo Lebrun]

IRQ_MASK and I2C_CLEAR_ALL_INTS are redundant. One masks the top three
bits off as reserved, the other one masks the reserved IRQs inside the
u32. Get rid of IRQ_MASK and only use the most restrictive mask.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index cd511c884f99..80bdf7e42613 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -94,9 +94,6 @@
 /* some bits in ICR are reserved */
 #define I2C_CLEAR_ALL_INTS	0x131f007f
 
-/* first three msb bits are reserved */
-#define IRQ_MASK(mask)		(mask & 0x1fffffff)
-
 /* maximum threshold value */
 #define MAX_I2C_FIFO_THRESHOLD	15
 
@@ -249,8 +246,7 @@ static int flush_i2c_fifo(struct nmk_i2c_dev *priv)
  */
 static void disable_all_interrupts(struct nmk_i2c_dev *priv)
 {
-	u32 mask = IRQ_MASK(0);
-	writel(mask, priv->virtbase + I2C_IMSCR);
+	writel(0, priv->virtbase + I2C_IMSCR);
 }
 
 /**
@@ -259,9 +255,7 @@ static void disable_all_interrupts(struct nmk_i2c_dev *priv)
  */
 static void clear_all_interrupts(struct nmk_i2c_dev *priv)
 {
-	u32 mask;
-	mask = IRQ_MASK(I2C_CLEAR_ALL_INTS);
-	writel(mask, priv->virtbase + I2C_ICR);
+	writel(I2C_CLEAR_ALL_INTS, priv->virtbase + I2C_ICR);
 }
 
 /**
@@ -468,7 +462,7 @@ static int read_i2c(struct nmk_i2c_dev *priv, u16 flags)
 	else
 		irq_mask |= I2C_IT_MTDWS;
 
-	irq_mask = I2C_CLEAR_ALL_INTS & IRQ_MASK(irq_mask);
+	irq_mask &= I2C_CLEAR_ALL_INTS;
 
 	writel(readl(priv->virtbase + I2C_IMSCR) | irq_mask,
 	       priv->virtbase + I2C_IMSCR);
@@ -547,7 +541,7 @@ static int write_i2c(struct nmk_i2c_dev *priv, u16 flags)
 	else
 		irq_mask |= I2C_IT_MTDWS;
 
-	irq_mask = I2C_CLEAR_ALL_INTS & IRQ_MASK(irq_mask);
+	irq_mask &= I2C_CLEAR_ALL_INTS;
 
 	writel(readl(priv->virtbase + I2C_IMSCR) | irq_mask,
 	       priv->virtbase + I2C_IMSCR);
@@ -703,8 +697,8 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
  */
 static int disable_interrupts(struct nmk_i2c_dev *priv, u32 irq)
 {
-	irq = IRQ_MASK(irq);
-	writel(readl(priv->virtbase + I2C_IMSCR) & ~(I2C_CLEAR_ALL_INTS & irq),
+	irq &= I2C_CLEAR_ALL_INTS;
+	writel(readl(priv->virtbase + I2C_IMSCR) & ~irq,
 	       priv->virtbase + I2C_IMSCR);
 	return 0;
 }

-- 
2.44.0

Re: [SPAM] [PATCH v2 04/11] i2c: nomadik: simplify IRQ masking logic

[Andi Shyti]

Hi Theo,

On Thu, Feb 29, 2024 at 07:10:52PM +0100, Théo Lebrun wrote:
> IRQ_MASK and I2C_CLEAR_ALL_INTS are redundant. One masks the top three

if I2C_CLEAR_ALL_INTS is redundant why don't you remove it?

> bits off as reserved, the other one masks the reserved IRQs inside the
> u32. Get rid of IRQ_MASK and only use the most restrictive mask.

Why is IRQ_MASK redundant? What happens if you write in the
reserved bits?

Can you please explain a bit better the change you did?

Thanks,
Andi

> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Re: [SPAM] [PATCH v2 04/11] i2c: nomadik: simplify IRQ masking logic

[Théo Lebrun]

Hello,

On Sat Mar 2, 2024 at 1:39 AM CET, Andi Shyti wrote:
> On Thu, Feb 29, 2024 at 07:10:52PM +0100, Théo Lebrun wrote:
> > IRQ_MASK and I2C_CLEAR_ALL_INTS are redundant. One masks the top three
>
> if I2C_CLEAR_ALL_INTS is redundant why don't you remove it?

I understand this is unclear. What I meant by redundant is that they are
redundant from one another; one overlaps the other. I'll give a better
commit description for v3. Something like:

   IRQ_MASK and I2C_CLEAR_ALL_INTS both mask available interrupts.
   IRQ_MASK removes top options (bits 29-31). I2C_CLEAR_ALL_INTS
   removes reserved options including top bits. Keep the latter.

   31  29  27  25  23  21  19  17  15  13  11  09  07  05  03  01
     30  28  26  24  22  20  18  16  14  12  10  08  06  04  02  00
   --- IRQ_MASK: --------------------------------------------------
          1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1
    0 0 0
   --- I2C_CLEAR_ALL_INTS: ----------------------------------------
          1     1 1       1 1 1 1 1                   1 1 1 1 1 1 1
    0 0 0   0 0     0 0 0           0 0 0 0 0 0 0 0 0

    Notice I2C_CLEAR_ALL_INTS is more restrictive than IRQ_MASK.

Is that better?

> > bits off as reserved, the other one masks the reserved IRQs inside the
> > u32. Get rid of IRQ_MASK and only use the most restrictive mask.
>
> Why is IRQ_MASK redundant? What happens if you write in the
> reserved bits?

The wording wasn't correct. Have I answered your
question from the above?

Thanks Andi,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v2 05/11] i2c: nomadik: use bitops helpers

[Théo Lebrun]

Constant register bit fields are declared using hardcoded hex values;
replace them by calls to BIT() and GENMASK(). Replace custom GEN_MASK()
macro by the generic FIELD_PREP(). Replace manual bit manipulations by
the generic FIELD_GET() macro.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 150 ++++++++++++++++++++-------------------
 1 file changed, 77 insertions(+), 73 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 80bdf7e42613..aa68ab402b10 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -9,6 +9,7 @@
  * Author: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com>
  * Author: Sachin Verma <sachin.verma@st.com>
  */
+#include <linux/bitfield.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/amba/bus.h>
@@ -42,54 +43,59 @@
 #define I2C_ICR		(0x038)
 
 /* Control registers */
-#define I2C_CR_PE		(0x1 << 0)	/* Peripheral Enable */
-#define I2C_CR_OM		(0x3 << 1)	/* Operating mode */
-#define I2C_CR_SAM		(0x1 << 3)	/* Slave addressing mode */
-#define I2C_CR_SM		(0x3 << 4)	/* Speed mode */
-#define I2C_CR_SGCM		(0x1 << 6)	/* Slave general call mode */
-#define I2C_CR_FTX		(0x1 << 7)	/* Flush Transmit */
-#define I2C_CR_FRX		(0x1 << 8)	/* Flush Receive */
-#define I2C_CR_DMA_TX_EN	(0x1 << 9)	/* DMA Tx enable */
-#define I2C_CR_DMA_RX_EN	(0x1 << 10)	/* DMA Rx Enable */
-#define I2C_CR_DMA_SLE		(0x1 << 11)	/* DMA sync. logic enable */
-#define I2C_CR_LM		(0x1 << 12)	/* Loopback mode */
-#define I2C_CR_FON		(0x3 << 13)	/* Filtering on */
-#define I2C_CR_FS		(0x3 << 15)	/* Force stop enable */
+#define I2C_CR_PE		BIT(0)		/* Peripheral Enable */
+#define I2C_CR_OM		GENMASK(2, 1)	/* Operating mode */
+#define I2C_CR_SAM		BIT(3)		/* Slave addressing mode */
+#define I2C_CR_SM		GENMASK(5, 4)	/* Speed mode */
+#define I2C_CR_SGCM		BIT(6)		/* Slave general call mode */
+#define I2C_CR_FTX		BIT(7)		/* Flush Transmit */
+#define I2C_CR_FRX		BIT(8)		/* Flush Receive */
+#define I2C_CR_DMA_TX_EN	BIT(9)		/* DMA Tx enable */
+#define I2C_CR_DMA_RX_EN	BIT(10)		/* DMA Rx Enable */
+#define I2C_CR_DMA_SLE		BIT(11)		/* DMA sync. logic enable */
+#define I2C_CR_LM		BIT(12)		/* Loopback mode */
+#define I2C_CR_FON		GENMASK(14, 13)	/* Filtering on */
+#define I2C_CR_FS		GENMASK(16, 15)	/* Force stop enable */
+
+/* Slave control register (SCR) */
+#define I2C_SCR_SLSU		GENMASK(31, 16)	/* Slave data setup time */
 
 /* Master controller (MCR) register */
-#define I2C_MCR_OP		(0x1 << 0)	/* Operation */
-#define I2C_MCR_A7		(0x7f << 1)	/* 7-bit address */
-#define I2C_MCR_EA10		(0x7 << 8)	/* 10-bit Extended address */
-#define I2C_MCR_SB		(0x1 << 11)	/* Extended address */
-#define I2C_MCR_AM		(0x3 << 12)	/* Address type */
-#define I2C_MCR_STOP		(0x1 << 14)	/* Stop condition */
-#define I2C_MCR_LENGTH		(0x7ff << 15)	/* Transaction length */
+#define I2C_MCR_OP		BIT(0)		/* Operation */
+#define I2C_MCR_A7		GENMASK(7, 1)	/* 7-bit address */
+#define I2C_MCR_EA10		GENMASK(10, 8)	/* 10-bit Extended address */
+#define I2C_MCR_SB		BIT(11)		/* Extended address */
+#define I2C_MCR_AM		GENMASK(13, 12)	/* Address type */
+#define I2C_MCR_STOP		BIT(14)		/* Stop condition */
+#define I2C_MCR_LENGTH		GENMASK(25, 15)	/* Transaction length */
 
 /* Status register (SR) */
-#define I2C_SR_OP		(0x3 << 0)	/* Operation */
-#define I2C_SR_STATUS		(0x3 << 2)	/* controller status */
-#define I2C_SR_CAUSE		(0x7 << 4)	/* Abort cause */
-#define I2C_SR_TYPE		(0x3 << 7)	/* Receive type */
-#define I2C_SR_LENGTH		(0x7ff << 9)	/* Transfer length */
+#define I2C_SR_OP		GENMASK(1, 0)	/* Operation */
+#define I2C_SR_STATUS		GENMASK(3, 2)	/* controller status */
+#define I2C_SR_CAUSE		GENMASK(6, 4)	/* Abort cause */
+#define I2C_SR_TYPE		GENMASK(8, 7)	/* Receive type */
+#define I2C_SR_LENGTH		GENMASK(19, 9)	/* Transfer length */
+
+/* Baud-rate counter register (BRCR) */
+#define I2C_BRCR_BRCNT1		GENMASK(31, 16)	/* Baud-rate counter 1 */
+#define I2C_BRCR_BRCNT2		GENMASK(15, 0)	/* Baud-rate counter 2 */
 
 /* Interrupt mask set/clear (IMSCR) bits */
-#define I2C_IT_TXFE		(0x1 << 0)
-#define I2C_IT_TXFNE		(0x1 << 1)
-#define I2C_IT_TXFF		(0x1 << 2)
-#define I2C_IT_TXFOVR		(0x1 << 3)
-#define I2C_IT_RXFE		(0x1 << 4)
-#define I2C_IT_RXFNF		(0x1 << 5)
-#define I2C_IT_RXFF		(0x1 << 6)
-#define I2C_IT_RFSR		(0x1 << 16)
-#define I2C_IT_RFSE		(0x1 << 17)
-#define I2C_IT_WTSR		(0x1 << 18)
-#define I2C_IT_MTD		(0x1 << 19)
-#define I2C_IT_STD		(0x1 << 20)
-#define I2C_IT_MAL		(0x1 << 24)
-#define I2C_IT_BERR		(0x1 << 25)
-#define I2C_IT_MTDWS		(0x1 << 28)
-
-#define GEN_MASK(val, mask, sb)  (((val) << (sb)) & (mask))
+#define I2C_IT_TXFE		BIT(0)
+#define I2C_IT_TXFNE		BIT(1)
+#define I2C_IT_TXFF		BIT(2)
+#define I2C_IT_TXFOVR		BIT(3)
+#define I2C_IT_RXFE		BIT(4)
+#define I2C_IT_RXFNF		BIT(5)
+#define I2C_IT_RXFF		BIT(6)
+#define I2C_IT_RFSR		BIT(16)
+#define I2C_IT_RFSE		BIT(17)
+#define I2C_IT_WTSR		BIT(18)
+#define I2C_IT_MTD		BIT(19)
+#define I2C_IT_STD		BIT(20)
+#define I2C_IT_MAL		BIT(24)
+#define I2C_IT_BERR		BIT(25)
+#define I2C_IT_MTDWS		BIT(28)
 
 /* some bits in ICR are reserved */
 #define I2C_CLEAR_ALL_INTS	0x131f007f
@@ -284,7 +290,7 @@ static int init_hw(struct nmk_i2c_dev *priv)
 }
 
 /* enable peripheral, master mode operation */
-#define DEFAULT_I2C_REG_CR	((1 << 1) | I2C_CR_PE)
+#define DEFAULT_I2C_REG_CR	(FIELD_PREP(I2C_CR_OM, 0b01) | I2C_CR_PE)
 
 /**
  * load_i2c_mcr_reg() - load the MCR register
@@ -296,41 +302,42 @@ static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *priv, u16 flags)
 	u32 mcr = 0;
 	unsigned short slave_adr_3msb_bits;
 
-	mcr |= GEN_MASK(priv->cli.slave_adr, I2C_MCR_A7, 1);
+	mcr |= FIELD_PREP(I2C_MCR_A7, priv->cli.slave_adr);
 
 	if (unlikely(flags & I2C_M_TEN)) {
 		/* 10-bit address transaction */
-		mcr |= GEN_MASK(2, I2C_MCR_AM, 12);
+		mcr |= FIELD_PREP(I2C_MCR_AM, 2);
 		/*
 		 * Get the top 3 bits.
 		 * EA10 represents extended address in MCR. This includes
 		 * the extension (MSB bits) of the 7 bit address loaded
 		 * in A7
 		 */
-		slave_adr_3msb_bits = (priv->cli.slave_adr >> 7) & 0x7;
+		slave_adr_3msb_bits = FIELD_GET(GENMASK(9, 7),
+						priv->cli.slave_adr);
 
-		mcr |= GEN_MASK(slave_adr_3msb_bits, I2C_MCR_EA10, 8);
+		mcr |= FIELD_PREP(I2C_MCR_EA10, slave_adr_3msb_bits);
 	} else {
 		/* 7-bit address transaction */
-		mcr |= GEN_MASK(1, I2C_MCR_AM, 12);
+		mcr |= FIELD_PREP(I2C_MCR_AM, 1);
 	}
 
 	/* start byte procedure not applied */
-	mcr |= GEN_MASK(0, I2C_MCR_SB, 11);
+	mcr |= FIELD_PREP(I2C_MCR_SB, 0);
 
 	/* check the operation, master read/write? */
 	if (priv->cli.operation == I2C_WRITE)
-		mcr |= GEN_MASK(I2C_WRITE, I2C_MCR_OP, 0);
+		mcr |= FIELD_PREP(I2C_MCR_OP, I2C_WRITE);
 	else
-		mcr |= GEN_MASK(I2C_READ, I2C_MCR_OP, 0);
+		mcr |= FIELD_PREP(I2C_MCR_OP, I2C_READ);
 
 	/* stop or repeated start? */
 	if (priv->stop)
-		mcr |= GEN_MASK(1, I2C_MCR_STOP, 14);
+		mcr |= FIELD_PREP(I2C_MCR_STOP, 1);
 	else
-		mcr &= ~(GEN_MASK(1, I2C_MCR_STOP, 14));
+		mcr &= ~FIELD_PREP(I2C_MCR_STOP, 1);
 
-	mcr |= GEN_MASK(priv->cli.count, I2C_MCR_LENGTH, 15);
+	mcr |= FIELD_PREP(I2C_MCR_LENGTH, priv->cli.count);
 
 	return mcr;
 }
@@ -383,7 +390,7 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
 	slsu += 1;
 
 	dev_dbg(&priv->adev->dev, "calculated SLSU = %04x\n", slsu);
-	writel(slsu << 16, priv->virtbase + I2C_SCR);
+	writel(FIELD_PREP(I2C_SCR_SLSU, slsu), priv->virtbase + I2C_SCR);
 
 	/*
 	 * The spec says, in case of std. mode the divider is
@@ -399,8 +406,8 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
 	 * plus operation. Currently we do not supprt high speed mode
 	 * so set brcr1 to 0.
 	 */
-	brcr1 = 0 << 16;
-	brcr2 = (i2c_clk / (priv->clk_freq * div)) & 0xffff;
+	brcr1 = FIELD_PREP(I2C_BRCR_BRCNT1, 0);
+	brcr2 = FIELD_PREP(I2C_BRCR_BRCNT2, i2c_clk / (priv->clk_freq * div));
 
 	/* set the baud rate counter register */
 	writel((brcr1 | brcr2), priv->virtbase + I2C_BRCR);
@@ -414,12 +421,13 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
 	if (priv->sm > I2C_FREQ_MODE_FAST) {
 		dev_err(&priv->adev->dev,
 			"do not support this mode defaulting to std. mode\n");
-		brcr2 = i2c_clk / (I2C_MAX_STANDARD_MODE_FREQ * 2) & 0xffff;
+		brcr2 = FIELD_PREP(I2C_BRCR_BRCNT2,
+				   i2c_clk / (I2C_MAX_STANDARD_MODE_FREQ * 2));
 		writel((brcr1 | brcr2), priv->virtbase + I2C_BRCR);
-		writel(I2C_FREQ_MODE_STANDARD << 4,
-				priv->virtbase + I2C_CR);
+		writel(FIELD_PREP(I2C_CR_SM, I2C_FREQ_MODE_STANDARD),
+		       priv->virtbase + I2C_CR);
 	}
-	writel(priv->sm << 4, priv->virtbase + I2C_CR);
+	writel(FIELD_PREP(I2C_CR_SM, priv->sm), priv->virtbase + I2C_CR);
 
 	/* set the Tx and Rx FIFO threshold */
 	writel(priv->tft, priv->virtbase + I2C_TFTR);
@@ -583,13 +591,8 @@ static int nmk_i2c_xfer_one(struct nmk_i2c_dev *priv, u16 flags)
 		u32 cause;
 
 		i2c_sr = readl(priv->virtbase + I2C_SR);
-		/*
-		 * Check if the controller I2C operation status
-		 * is set to ABORT(11b).
-		 */
-		if (((i2c_sr >> 2) & 0x3) == 0x3) {
-			/* get the abort cause */
-			cause =	(i2c_sr >> 4) & 0x7;
+		if (FIELD_GET(I2C_SR_STATUS, i2c_sr) == I2C_ABORT) {
+			cause = FIELD_GET(I2C_SR_CAUSE, i2c_sr);
 			dev_err(&priv->adev->dev, "%s\n",
 				cause >= ARRAY_SIZE(abort_causes) ?
 				"unknown reason" :
@@ -730,7 +733,7 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	misr = readl(priv->virtbase + I2C_MISR);
 
 	src = __ffs(misr);
-	switch ((1 << src)) {
+	switch (BIT(src)) {
 
 	/* Transmit FIFO nearly empty interrupt */
 	case I2C_IT_TXFNE:
@@ -824,15 +827,16 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	 * during the transaction.
 	 */
 	case I2C_IT_BERR:
+	{
+		u32 sr = readl(priv->virtbase + I2C_SR);
 		priv->result = -EIO;
-		/* get the status */
-		if (((readl(priv->virtbase + I2C_SR) >> 2) & 0x3) == I2C_ABORT)
+		if (FIELD_GET(I2C_SR_STATUS, sr) == I2C_ABORT)
 			init_hw(priv);
 
 		i2c_set_bit(priv->virtbase + I2C_ICR, I2C_IT_BERR);
 		complete(&priv->xfer_complete);
-
-		break;
+	}
+	break;
 
 	/*
 	 * Tx FIFO overrun interrupt.

-- 
2.44.0

Re: [PATCH v2 05/11] i2c: nomadik: use bitops helpers

[Andi Shyti]

Hi Theo,

...

> @@ -284,7 +290,7 @@ static int init_hw(struct nmk_i2c_dev *priv)
>  }
>  
>  /* enable peripheral, master mode operation */
> -#define DEFAULT_I2C_REG_CR	((1 << 1) | I2C_CR_PE)
> +#define DEFAULT_I2C_REG_CR	(FIELD_PREP(I2C_CR_OM, 0b01) | I2C_CR_PE)

0b01?

>  /**
>   * load_i2c_mcr_reg() - load the MCR register
> @@ -296,41 +302,42 @@ static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *priv, u16 flags)
>  	u32 mcr = 0;
>  	unsigned short slave_adr_3msb_bits;
>  
> -	mcr |= GEN_MASK(priv->cli.slave_adr, I2C_MCR_A7, 1);
> +	mcr |= FIELD_PREP(I2C_MCR_A7, priv->cli.slave_adr);
>  
>  	if (unlikely(flags & I2C_M_TEN)) {
>  		/* 10-bit address transaction */
> -		mcr |= GEN_MASK(2, I2C_MCR_AM, 12);
> +		mcr |= FIELD_PREP(I2C_MCR_AM, 2);
>  		/*
>  		 * Get the top 3 bits.
>  		 * EA10 represents extended address in MCR. This includes
>  		 * the extension (MSB bits) of the 7 bit address loaded
>  		 * in A7
>  		 */
> -		slave_adr_3msb_bits = (priv->cli.slave_adr >> 7) & 0x7;
> +		slave_adr_3msb_bits = FIELD_GET(GENMASK(9, 7),
> +						priv->cli.slave_adr);

This looks like the only one not having a define. Shall we give a
definition to GENMASK(9, 7)?

>  
> -		mcr |= GEN_MASK(slave_adr_3msb_bits, I2C_MCR_EA10, 8);
> +		mcr |= FIELD_PREP(I2C_MCR_EA10, slave_adr_3msb_bits);

...

> @@ -824,15 +827,16 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
>  	 * during the transaction.
>  	 */
>  	case I2C_IT_BERR:
> +	{
> +		u32 sr = readl(priv->virtbase + I2C_SR);

please give a blank line after the variable definition.

Besides, I'd prefer the assignment, when it is a bit more
complex, in a different line, as well.

Rest looks OK, didn't see anything off.

Andi

Re: [PATCH v2 05/11] i2c: nomadik: use bitops helpers

[Théo Lebrun]

Hello,

On Sat Mar 2, 2024 at 2:31 AM CET, Andi Shyti wrote:

[...]

> > @@ -284,7 +290,7 @@ static int init_hw(struct nmk_i2c_dev *priv)
> >  }
> >  
> >  /* enable peripheral, master mode operation */
> > -#define DEFAULT_I2C_REG_CR	((1 << 1) | I2C_CR_PE)
> > +#define DEFAULT_I2C_REG_CR	(FIELD_PREP(I2C_CR_OM, 0b01) | I2C_CR_PE)
>
> 0b01?

OM is a two-bit field. We want to put 0b01 in that. We do not declare
constants for its 4 potential values. Values are:

 - 0b00 slave mode
 - 0b01 master mode
 - 0b10 master/slave mode
 - 0b11 reserved

To me the comment above DEFAULT_I2C_REG_CR is enough to show that we go
into master mode. We could declare all values as constants but only
0b01 would get used.

>
> >  /**
> >   * load_i2c_mcr_reg() - load the MCR register
> > @@ -296,41 +302,42 @@ static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *priv, u16 flags)
> >  	u32 mcr = 0;
> >  	unsigned short slave_adr_3msb_bits;
> >  
> > -	mcr |= GEN_MASK(priv->cli.slave_adr, I2C_MCR_A7, 1);
> > +	mcr |= FIELD_PREP(I2C_MCR_A7, priv->cli.slave_adr);
> >  
> >  	if (unlikely(flags & I2C_M_TEN)) {
> >  		/* 10-bit address transaction */
> > -		mcr |= GEN_MASK(2, I2C_MCR_AM, 12);
> > +		mcr |= FIELD_PREP(I2C_MCR_AM, 2);
> >  		/*
> >  		 * Get the top 3 bits.
> >  		 * EA10 represents extended address in MCR. This includes
> >  		 * the extension (MSB bits) of the 7 bit address loaded
> >  		 * in A7
> >  		 */
> > -		slave_adr_3msb_bits = (priv->cli.slave_adr >> 7) & 0x7;
> > +		slave_adr_3msb_bits = FIELD_GET(GENMASK(9, 7),
> > +						priv->cli.slave_adr);
>
> This looks like the only one not having a define. Shall we give a
> definition to GENMASK(9, 7)?

Indeed. What should it be named? It could be generic; this is about
getting the upper 3 bits from an extended (10-bit) I2C address?

> > -		mcr |= GEN_MASK(slave_adr_3msb_bits, I2C_MCR_EA10, 8);
> > +		mcr |= FIELD_PREP(I2C_MCR_EA10, slave_adr_3msb_bits);
>
> ...
>
> > @@ -824,15 +827,16 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
> >  	 * during the transaction.
> >  	 */
> >  	case I2C_IT_BERR:
> > +	{
> > +		u32 sr = readl(priv->virtbase + I2C_SR);
>
> please give a blank line after the variable definition.
>
> Besides, I'd prefer the assignment, when it is a bit more
> complex, in a different line, as well.

Will do.

> Rest looks OK, didn't see anything off.

Thanks for the review Andi!

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v2 06/11] i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer

[Théo Lebrun]

Replace the completion by a waitqueue for synchronization from IRQ
handler to task. For short timeouts, use hrtimers, else use timers.
Usecase: avoid blocking the I2C bus for too long when an issue occurs.

The threshold picked is one jiffy: if timeout is below that, use
hrtimers. This threshold is NOT configurable.

Implement behavior but do NOT change fetching of timeout. This means the
timeout is unchanged (200ms) and the hrtimer case will never trigger.

A waitqueue is used because it supports both desired timeout approaches.
See wait_event_timeout() and wait_event_hrtimeout(). An atomic boolean
serves as synchronization condition.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 70 +++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 22 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index aa68ab402b10..e68b8e0d7919 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -162,10 +162,11 @@ struct i2c_nmk_client {
  * @clk_freq: clock frequency for the operation mode
  * @tft: Tx FIFO Threshold in bytes
  * @rft: Rx FIFO Threshold in bytes
- * @timeout: Slave response timeout (ms)
+ * @timeout_usecs: Slave response timeout
  * @sm: speed mode
  * @stop: stop condition.
- * @xfer_complete: acknowledge completion for a I2C message.
+ * @xfer_wq: xfer done wait queue.
+ * @xfer_done: xfer done boolean.
  * @result: controller propogated result.
  */
 struct nmk_i2c_dev {
@@ -179,10 +180,11 @@ struct nmk_i2c_dev {
 	u32				clk_freq;
 	unsigned char			tft;
 	unsigned char			rft;
-	int				timeout;
+	int				timeout_usecs;
 	enum i2c_freq_mode		sm;
 	int				stop;
-	struct completion		xfer_complete;
+	struct wait_queue_head		xfer_wq;
+	bool				xfer_done;
 	int				result;
 };
 
@@ -434,6 +436,22 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
 	writel(priv->rft, priv->virtbase + I2C_RFTR);
 }
 
+static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
+{
+	if (priv->timeout_usecs < jiffies_to_usecs(1)) {
+		unsigned long timeout_usecs = priv->timeout_usecs;
+		ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC);
+
+		wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout);
+	} else {
+		unsigned long timeout = usecs_to_jiffies(priv->timeout_usecs);
+
+		wait_event_timeout(priv->xfer_wq, priv->xfer_done, timeout);
+	}
+
+	return priv->xfer_done;
+}
+
 /**
  * read_i2c() - Read from I2C client device
  * @priv: private data of I2C Driver
@@ -445,9 +463,9 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
  */
 static int read_i2c(struct nmk_i2c_dev *priv, u16 flags)
 {
-	int status = 0;
 	u32 mcr, irq_mask;
-	unsigned long timeout;
+	int status = 0;
+	bool xfer_done;
 
 	mcr = load_i2c_mcr_reg(priv, flags);
 	writel(mcr, priv->virtbase + I2C_MCR);
@@ -459,7 +477,8 @@ static int read_i2c(struct nmk_i2c_dev *priv, u16 flags)
 	/* enable the controller */
 	i2c_set_bit(priv->virtbase + I2C_CR, I2C_CR_PE);
 
-	init_completion(&priv->xfer_complete);
+	init_waitqueue_head(&priv->xfer_wq);
+	priv->xfer_done = false;
 
 	/* enable interrupts by setting the mask */
 	irq_mask = (I2C_IT_RXFNF | I2C_IT_RXFF |
@@ -475,10 +494,9 @@ static int read_i2c(struct nmk_i2c_dev *priv, u16 flags)
 	writel(readl(priv->virtbase + I2C_IMSCR) | irq_mask,
 	       priv->virtbase + I2C_IMSCR);
 
-	timeout = wait_for_completion_timeout(
-		&priv->xfer_complete, priv->adap.timeout);
+	xfer_done = nmk_i2c_wait_xfer_done(priv);
 
-	if (timeout == 0) {
+	if (!xfer_done) {
 		/* Controller timed out */
 		dev_err(&priv->adev->dev, "read from slave 0x%x timed out\n",
 			priv->cli.slave_adr);
@@ -513,9 +531,9 @@ static void fill_tx_fifo(struct nmk_i2c_dev *priv, int no_bytes)
  */
 static int write_i2c(struct nmk_i2c_dev *priv, u16 flags)
 {
-	u32 status = 0;
 	u32 mcr, irq_mask;
-	unsigned long timeout;
+	u32 status = 0;
+	bool xfer_done;
 
 	mcr = load_i2c_mcr_reg(priv, flags);
 
@@ -528,7 +546,8 @@ static int write_i2c(struct nmk_i2c_dev *priv, u16 flags)
 	/* enable the controller */
 	i2c_set_bit(priv->virtbase + I2C_CR, I2C_CR_PE);
 
-	init_completion(&priv->xfer_complete);
+	init_waitqueue_head(&priv->xfer_wq);
+	priv->xfer_done = false;
 
 	/* enable interrupts by settings the masks */
 	irq_mask = (I2C_IT_TXFOVR | I2C_IT_MAL | I2C_IT_BERR);
@@ -554,10 +573,9 @@ static int write_i2c(struct nmk_i2c_dev *priv, u16 flags)
 	writel(readl(priv->virtbase + I2C_IMSCR) | irq_mask,
 	       priv->virtbase + I2C_IMSCR);
 
-	timeout = wait_for_completion_timeout(
-		&priv->xfer_complete, priv->adap.timeout);
+	xfer_done = nmk_i2c_wait_xfer_done(priv);
 
-	if (timeout == 0) {
+	if (!xfer_done) {
 		/* Controller timed out */
 		dev_err(&priv->adev->dev, "write to slave 0x%x timed out\n",
 			priv->cli.slave_adr);
@@ -807,7 +825,9 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 				priv->cli.count);
 			init_hw(priv);
 		}
-		complete(&priv->xfer_complete);
+		priv->xfer_done = true;
+		wake_up(&priv->xfer_wq);
+
 
 		break;
 
@@ -817,7 +837,9 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 		init_hw(priv);
 
 		i2c_set_bit(priv->virtbase + I2C_ICR, I2C_IT_MAL);
-		complete(&priv->xfer_complete);
+		priv->xfer_done = true;
+		wake_up(&priv->xfer_wq);
+
 
 		break;
 
@@ -834,7 +856,9 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 			init_hw(priv);
 
 		i2c_set_bit(priv->virtbase + I2C_ICR, I2C_IT_BERR);
-		complete(&priv->xfer_complete);
+		priv->xfer_done = true;
+		wake_up(&priv->xfer_wq);
+
 	}
 	break;
 
@@ -848,7 +872,9 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 		init_hw(priv);
 
 		dev_err(dev, "Tx Fifo Over run\n");
-		complete(&priv->xfer_complete);
+		priv->xfer_done = true;
+		wake_up(&priv->xfer_wq);
+
 
 		break;
 
@@ -949,7 +975,7 @@ static void nmk_i2c_of_probe(struct device_node *np,
 		priv->sm = I2C_FREQ_MODE_FAST;
 	priv->tft = 1; /* Tx FIFO threshold */
 	priv->rft = 8; /* Rx FIFO threshold */
-	priv->timeout = 200; /* Slave response timeout(ms) */
+	priv->timeout_usecs = 200 * USEC_PER_MSEC; /* Slave response timeout */
 }
 
 static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
@@ -1009,7 +1035,7 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 	adap->owner = THIS_MODULE;
 	adap->class = I2C_CLASS_DEPRECATED;
 	adap->algo = &nmk_i2c_algo;
-	adap->timeout = msecs_to_jiffies(priv->timeout);
+	adap->timeout = usecs_to_jiffies(priv->timeout_usecs);
 	snprintf(adap->name, sizeof(adap->name),
 		 "Nomadik I2C at %pR", &adev->res);
 

-- 
2.44.0

Re: [PATCH v2 06/11] i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer

[Wolfram Sang]

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

On Thu, Feb 29, 2024 at 07:10:54PM +0100, Théo Lebrun wrote:
> Replace the completion by a waitqueue for synchronization from IRQ
> handler to task. For short timeouts, use hrtimers, else use timers.
> Usecase: avoid blocking the I2C bus for too long when an issue occurs.
> 
> The threshold picked is one jiffy: if timeout is below that, use
> hrtimers. This threshold is NOT configurable.
> 
> Implement behavior but do NOT change fetching of timeout. This means the
> timeout is unchanged (200ms) and the hrtimer case will never trigger.
> 
> A waitqueue is used because it supports both desired timeout approaches.
> See wait_event_timeout() and wait_event_hrtimeout(). An atomic boolean
> serves as synchronization condition.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Largely:

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Nit:

> -	int				timeout;
> +	int				timeout_usecs;

I think 'unsigned' makes a lot of sense here. Maybe u32 even?


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

Re: [PATCH v2 06/11] i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer

[Théo Lebrun]

Hello,

On Mon Mar 4, 2024 at 10:18 AM CET, Wolfram Sang wrote:
> On Thu, Feb 29, 2024 at 07:10:54PM +0100, Théo Lebrun wrote:
> > Replace the completion by a waitqueue for synchronization from IRQ
> > handler to task. For short timeouts, use hrtimers, else use timers.
> > Usecase: avoid blocking the I2C bus for too long when an issue occurs.
> > 
> > The threshold picked is one jiffy: if timeout is below that, use
> > hrtimers. This threshold is NOT configurable.
> > 
> > Implement behavior but do NOT change fetching of timeout. This means the
> > timeout is unchanged (200ms) and the hrtimer case will never trigger.
> > 
> > A waitqueue is used because it supports both desired timeout approaches.
> > See wait_event_timeout() and wait_event_hrtimeout(). An atomic boolean
> > serves as synchronization condition.
> > 
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>
> Largely:
>
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for the reviews Wolfram.

> Nit:
>
> > -	int				timeout;
> > +	int				timeout_usecs;
>
> I think 'unsigned' makes a lot of sense here. Maybe u32 even?

Yes unsigned would make sense. unsigned int or u32, I wouldn't know
which to pick.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 06/11] i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer

[Wolfram Sang]

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


> > > -	int				timeout;
> > > +	int				timeout_usecs;
> >
> > I think 'unsigned' makes a lot of sense here. Maybe u32 even?
> 
> Yes unsigned would make sense. unsigned int or u32, I wouldn't know
> which to pick.

It gets (later) fed by of_property_read_u32(), so I tend to suggest u32.
Sounds like least conversions then but please double check.


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

Re: [SPAM] [PATCH v2 06/11] i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer

[Andi Shyti]

Hi Theo,

...

> +static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
> +{
> +	if (priv->timeout_usecs < jiffies_to_usecs(1)) {
> +		unsigned long timeout_usecs = priv->timeout_usecs;
> +		ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC);
> +
> +		wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout);
> +	} else {
> +		unsigned long timeout = usecs_to_jiffies(priv->timeout_usecs);
> +
> +		wait_event_timeout(priv->xfer_wq, priv->xfer_done, timeout);
> +	}
> +
> +	return priv->xfer_done;

You could eventually write this as

  static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
  {
	if (priv->timeout_usecs < jiffies_to_usecs(1)) {
		...

		return !wait_event_hrtimeout(...);
	}

	...
	return wait_event_timeout(...);
  }

It looks a bit cleaner to me... your choice.

Rest looks good.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Andi

Re: [SPAM] [PATCH v2 06/11] i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer

[Théo Lebrun]

Hello,

On Mon Mar 4, 2024 at 2:54 PM CET, Andi Shyti wrote:
> Hi Theo,
>
> ...
>
> > +static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
> > +{
> > +	if (priv->timeout_usecs < jiffies_to_usecs(1)) {
> > +		unsigned long timeout_usecs = priv->timeout_usecs;
> > +		ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC);
> > +
> > +		wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout);
> > +	} else {
> > +		unsigned long timeout = usecs_to_jiffies(priv->timeout_usecs);
> > +
> > +		wait_event_timeout(priv->xfer_wq, priv->xfer_done, timeout);
> > +	}
> > +
> > +	return priv->xfer_done;
>
> You could eventually write this as
>
>   static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
>   {
> 	if (priv->timeout_usecs < jiffies_to_usecs(1)) {
> 		...
>
> 		return !wait_event_hrtimeout(...);
> 	}
>
> 	...
> 	return wait_event_timeout(...);
>   }
>
> It looks a bit cleaner to me... your choice.

The full block would become:

static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
{
	if (priv->timeout_usecs < jiffies_to_usecs(1)) {
		unsigned long timeout_usecs = priv->timeout_usecs;
		ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC);

		return !wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done,
					     timeout);
	}

	return wait_event_timeout(priv->xfer_wq, priv->xfer_done,
				  usecs_to_jiffies(priv->timeout_usecs));
}

Three things:

 - Deindenting the jiffy timeout case means no variable declaration
   after the if-block. This is fine from my point-of-view.

 - It means we depend on the half-mess that are return values from
   wait_event_*timeout() macros. I wanted to avoid that because it
   looks like an error when you read the above code and see one is
   negated while the other is not.

 - Also, I'm not confident in casting either return value to bool; what
   happens if either macro returns an error? This is a theoretical case
   that shouldn't happen, but behavior might change at some point or
   bugs could occur. We know priv->xfer_done will give us the right
   answer.

My preference still goes to the original version, but I'm happy we are
having a discussion about this code block.

> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Thanks for your review Andi!

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: Re: [PATCH v2 06/11] i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer

[Andi Shyti]

Hi Theo,

On Mon, Mar 04, 2024 at 03:32:38PM +0100, Théo Lebrun wrote:
> On Mon Mar 4, 2024 at 2:54 PM CET, Andi Shyti wrote:
> > > +static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
> > > +{
> > > +	if (priv->timeout_usecs < jiffies_to_usecs(1)) {
> > > +		unsigned long timeout_usecs = priv->timeout_usecs;
> > > +		ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC);
> > > +
> > > +		wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout);
> > > +	} else {
> > > +		unsigned long timeout = usecs_to_jiffies(priv->timeout_usecs);
> > > +
> > > +		wait_event_timeout(priv->xfer_wq, priv->xfer_done, timeout);
> > > +	}
> > > +
> > > +	return priv->xfer_done;
> >
> > You could eventually write this as
> >
> >   static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
> >   {
> > 	if (priv->timeout_usecs < jiffies_to_usecs(1)) {
> > 		...
> >
> > 		return !wait_event_hrtimeout(...);
> > 	}
> >
> > 	...
> > 	return wait_event_timeout(...);
> >   }
> >
> > It looks a bit cleaner to me... your choice.
> 
> The full block would become:
> 
> static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv)
> {
> 	if (priv->timeout_usecs < jiffies_to_usecs(1)) {
> 		unsigned long timeout_usecs = priv->timeout_usecs;
> 		ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC);
> 
> 		return !wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done,
> 					     timeout);
> 	}
> 
> 	return wait_event_timeout(priv->xfer_wq, priv->xfer_done,
> 				  usecs_to_jiffies(priv->timeout_usecs));
> }
> 
> Three things:
> 
>  - Deindenting the jiffy timeout case means no variable declaration
>    after the if-block. This is fine from my point-of-view.
> 
>  - It means we depend on the half-mess that are return values from
>    wait_event_*timeout() macros. I wanted to avoid that because it
>    looks like an error when you read the above code and see one is
>    negated while the other is not.
> 
>  - Also, I'm not confident in casting either return value to bool; what
>    happens if either macro returns an error? This is a theoretical case
>    that shouldn't happen, but behavior might change at some point or
>    bugs could occur. We know priv->xfer_done will give us the right
>    answer.
> 
> My preference still goes to the original version, but I'm happy we are
> having a discussion about this code block.

sure... it's not a binding comment.

Andi

[PATCH v2 07/11] i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout

[Théo Lebrun]

The FIFO flush function uses a jiffies amount to detect timeouts as the
flushing is async. Replace with ktime to get more accurate precision
and support short timeouts.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index e68b8e0d7919..afd54999bbbb 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -219,8 +219,8 @@ static inline void i2c_clr_bit(void __iomem *reg, u32 mask)
 static int flush_i2c_fifo(struct nmk_i2c_dev *priv)
 {
 #define LOOP_ATTEMPTS 10
+	ktime_t timeout;
 	int i;
-	unsigned long timeout;
 
 	/*
 	 * flush the transmit and receive FIFO. The flushing
@@ -232,9 +232,9 @@ static int flush_i2c_fifo(struct nmk_i2c_dev *priv)
 	writel((I2C_CR_FTX | I2C_CR_FRX), priv->virtbase + I2C_CR);
 
 	for (i = 0; i < LOOP_ATTEMPTS; i++) {
-		timeout = jiffies + priv->adap.timeout;
+		timeout = ktime_add_us(ktime_get(), priv->timeout_usecs);
 
-		while (!time_after(jiffies, timeout)) {
+		while (ktime_after(timeout, ktime_get())) {
 			if ((readl(priv->virtbase + I2C_CR) &
 				(I2C_CR_FTX | I2C_CR_FRX)) == 0)
 				return 0;

-- 
2.44.0

Re: [PATCH v2 07/11] i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout

[Wolfram Sang]

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

On Thu, Feb 29, 2024 at 07:10:55PM +0100, Théo Lebrun wrote:
> The FIFO flush function uses a jiffies amount to detect timeouts as the
> flushing is async. Replace with ktime to get more accurate precision
> and support short timeouts.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


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

Re: [PATCH v2 07/11] i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout

[Andi Shyti]

Hi Theo,

On Thu, Feb 29, 2024 at 07:10:55PM +0100, Théo Lebrun wrote:
> The FIFO flush function uses a jiffies amount to detect timeouts as the
> flushing is async. Replace with ktime to get more accurate precision
> and support short timeouts.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Thanks,
Andi

[PATCH v2 08/11] i2c: nomadik: fetch i2c-transfer-timeout-us property from devicetree

[Théo Lebrun]

Allow overriding the default timeout value (200ms) from devicetree,
using the generic i2c-transfer-timeout-us property.

The i2c_adapter->timeout field is an unaccurate jiffies amount;
i2c-nomadik uses hrtimers for timeouts below one jiffy.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index afd54999bbbb..2d3247979e45 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -964,6 +964,8 @@ static const struct i2c_algorithm nmk_i2c_algo = {
 static void nmk_i2c_of_probe(struct device_node *np,
 			     struct nmk_i2c_dev *priv)
 {
+	u32 timeout_usecs;
+
 	/* Default to 100 kHz if no frequency is given in the node */
 	if (of_property_read_u32(np, "clock-frequency", &priv->clk_freq))
 		priv->clk_freq = I2C_MAX_STANDARD_MODE_FREQ;
@@ -975,7 +977,12 @@ static void nmk_i2c_of_probe(struct device_node *np,
 		priv->sm = I2C_FREQ_MODE_FAST;
 	priv->tft = 1; /* Tx FIFO threshold */
 	priv->rft = 8; /* Rx FIFO threshold */
-	priv->timeout_usecs = 200 * USEC_PER_MSEC; /* Slave response timeout */
+
+	/* Slave response timeout */
+	if (!of_property_read_u32(np, "i2c-transfer-timeout-us", &timeout_usecs))
+		priv->timeout_usecs = timeout_usecs;
+	else
+		priv->timeout_usecs = 200 * USEC_PER_MSEC;
 }
 
 static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)

-- 
2.44.0

Re: [PATCH v2 08/11] i2c: nomadik: fetch i2c-transfer-timeout-us property from devicetree

[Linus Walleij]

On Thu, Feb 29, 2024 at 7:10 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> Allow overriding the default timeout value (200ms) from devicetree,
> using the generic i2c-transfer-timeout-us property.
>
> The i2c_adapter->timeout field is an unaccurate jiffies amount;
> i2c-nomadik uses hrtimers for timeouts below one jiffy.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 08/11] i2c: nomadik: fetch i2c-transfer-timeout-us property from devicetree

[Wolfram Sang]

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

On Thu, Feb 29, 2024 at 07:10:56PM +0100, Théo Lebrun wrote:
> Allow overriding the default timeout value (200ms) from devicetree,
> using the generic i2c-transfer-timeout-us property.
> 
> The i2c_adapter->timeout field is an unaccurate jiffies amount;
> i2c-nomadik uses hrtimers for timeouts below one jiffy.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

I agree to get the DT property here in the driver. We may later refactor
it to handle it in the I2C core. Syncing this new property with the
existing 'adapter->timeout' will be a tad annoying, though. But I guess
the shorter timeouts are a desired feature these days...

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


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

Re: [PATCH v2 08/11] i2c: nomadik: fetch i2c-transfer-timeout-us property from devicetree

[Andi Shyti]

Hi Theo,

On Thu, Feb 29, 2024 at 07:10:56PM +0100, Théo Lebrun wrote:
> Allow overriding the default timeout value (200ms) from devicetree,
> using the generic i2c-transfer-timeout-us property.
> 
> The i2c_adapter->timeout field is an unaccurate jiffies amount;
> i2c-nomadik uses hrtimers for timeouts below one jiffy.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Thanks,
Andi

[PATCH v2 09/11] i2c: nomadik: support Mobileye EyeQ5 I2C controller

[Théo Lebrun]

Add compatible for the integration of the same DB8500 IP block into the
Mobileye EyeQ5 platform. Two quirks are present:

 - The memory bus only supports 32-bit accesses. Avoid writeb() and
   readb() by introducing helper functions that fallback to writel()
   and readl().

 - A register must be configured for the I2C speed mode; it is located
   in a shared register region called OLB. We access that memory region
   using a syscon & regmap that gets passed as a phandle (mobileye,olb).

   A two-bit enum per controller is written into the register; that
   requires us to know the global index of the I2C controller (cell arg
   to the mobileye,olb phandle).

We add #include <linux/mfd/syscon.h> and <linux/regmap.h> and sort
headers.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 95 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 84 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 2d3247979e45..e9a77377add4 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -6,22 +6,30 @@
  * I2C master mode controller driver, used in Nomadik 8815
  * and Ux500 platforms.
  *
+ * The Mobileye EyeQ5 platform is also supported; it uses
+ * the same Ux500/DB8500 IP block with two quirks:
+ *  - The memory bus only supports 32-bit accesses.
+ *  - A register must be configured for the I2C speed mode;
+ *    it is located in a shared register region called OLB.
+ *
  * Author: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com>
  * Author: Sachin Verma <sachin.verma@st.com>
  */
+#include <linux/amba/bus.h>
 #include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
 #include <linux/init.h>
-#include <linux/module.h>
-#include <linux/amba/bus.h>
-#include <linux/slab.h>
 #include <linux/interrupt.h>
-#include <linux/i2c.h>
-#include <linux/err.h>
-#include <linux/clk.h>
 #include <linux/io.h>
-#include <linux/pm_runtime.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
 
 #define DRIVER_NAME "nmk-i2c"
 
@@ -110,6 +118,10 @@ enum i2c_freq_mode {
 	I2C_FREQ_MODE_FAST_PLUS,	/* up to 1 Mb/s */
 };
 
+/* Mobileye EyeQ5 offset into a shared register region (called OLB) */
+#define NMK_I2C_EYEQ5_OLB_IOCR2			0x0B8
+#define NMK_I2C_EYEQ5_OLB_IOCR2_SHIFT(id)	(4 + 2 * (id))
+
 /**
  * struct i2c_vendor_data - per-vendor variations
  * @has_mtdws: variant has the MTDWS bit
@@ -168,6 +180,7 @@ struct i2c_nmk_client {
  * @xfer_wq: xfer done wait queue.
  * @xfer_done: xfer done boolean.
  * @result: controller propogated result.
+ * @has_32b_bus: controller is on a bus that only supports 32-bit accesses.
  */
 struct nmk_i2c_dev {
 	struct i2c_vendor_data		*vendor;
@@ -186,6 +199,7 @@ struct nmk_i2c_dev {
 	struct wait_queue_head		xfer_wq;
 	bool				xfer_done;
 	int				result;
+	bool				has_32b_bus;
 };
 
 /* controller's abort causes */
@@ -209,6 +223,24 @@ static inline void i2c_clr_bit(void __iomem *reg, u32 mask)
 	writel(readl(reg) & ~mask, reg);
 }
 
+static inline u8 nmk_i2c_readb(const struct nmk_i2c_dev *priv,
+			       unsigned long reg)
+{
+	if (priv->has_32b_bus)
+		return readl(priv->virtbase + reg);
+	else
+		return readb(priv->virtbase + reg);
+}
+
+static inline void nmk_i2c_writeb(const struct nmk_i2c_dev *priv, u32 val,
+				unsigned long reg)
+{
+	if (priv->has_32b_bus)
+		writel(val, priv->virtbase + reg);
+	else
+		writeb(val, priv->virtbase + reg);
+}
+
 /**
  * flush_i2c_fifo() - This function flushes the I2C FIFO
  * @priv: private data of I2C Driver
@@ -514,7 +546,7 @@ static void fill_tx_fifo(struct nmk_i2c_dev *priv, int no_bytes)
 			(priv->cli.count != 0);
 			count--) {
 		/* write to the Tx FIFO */
-		writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
+		nmk_i2c_writeb(priv, *priv->cli.buffer, I2C_TFR);
 		priv->cli.buffer++;
 		priv->cli.count--;
 		priv->cli.xfer_bytes++;
@@ -783,7 +815,7 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	case I2C_IT_RXFNF:
 		for (count = rft; count > 0; count--) {
 			/* Read the Rx FIFO */
-			*priv->cli.buffer = readb(priv->virtbase + I2C_RFR);
+			*priv->cli.buffer = nmk_i2c_readb(priv, I2C_RFR);
 			priv->cli.buffer++;
 		}
 		priv->cli.count -= rft;
@@ -793,7 +825,7 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	/* Rx FIFO full */
 	case I2C_IT_RXFF:
 		for (count = MAX_I2C_FIFO_THRESHOLD; count > 0; count--) {
-			*priv->cli.buffer = readb(priv->virtbase + I2C_RFR);
+			*priv->cli.buffer = nmk_i2c_readb(priv, I2C_RFR);
 			priv->cli.buffer++;
 		}
 		priv->cli.count -= MAX_I2C_FIFO_THRESHOLD;
@@ -809,7 +841,7 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 				if (priv->cli.count == 0)
 					break;
 				*priv->cli.buffer =
-					readb(priv->virtbase + I2C_RFR);
+					nmk_i2c_readb(priv, I2C_RFR);
 				priv->cli.buffer++;
 				priv->cli.count--;
 				priv->cli.xfer_bytes++;
@@ -985,6 +1017,38 @@ static void nmk_i2c_of_probe(struct device_node *np,
 		priv->timeout_usecs = 200 * USEC_PER_MSEC;
 }
 
+static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
+{
+	struct device *dev = &priv->adev->dev;
+	struct device_node *np = dev->of_node;
+	unsigned int shift, speed_mode;
+	struct regmap *olb;
+	unsigned int id;
+
+	priv->has_32b_bus = true;
+
+	olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id);
+	if (IS_ERR_OR_NULL(olb)) {
+		if (!olb)
+			olb = ERR_PTR(-ENOENT);
+		return dev_err_probe(dev, PTR_ERR(olb),
+				     "failed OLB lookup: %lu\n", PTR_ERR(olb));
+	}
+
+	if (priv->clk_freq <= 400000)
+		speed_mode = 0b00;
+	else if (priv->clk_freq <= 1000000)
+		speed_mode = 0b01;
+	else
+		speed_mode = 0b10;
+
+	shift = NMK_I2C_EYEQ5_OLB_IOCR2_SHIFT(id);
+	regmap_update_bits(olb, NMK_I2C_EYEQ5_OLB_IOCR2,
+			   0b11 << shift, speed_mode << shift);
+
+	return 0;
+}
+
 static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 {
 	int ret = 0;
@@ -1001,8 +1065,17 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 
 	priv->vendor = vendor;
 	priv->adev = adev;
+	priv->has_32b_bus = false;
 	nmk_i2c_of_probe(np, priv);
 
+	if (of_device_is_compatible(np, "mobileye,eyeq5-i2c")) {
+		ret = nmk_i2c_eyeq5_probe(priv);
+		if (ret) {
+			dev_info(dev, "%s: %d: %d\n", __func__, __LINE__, ret);
+			return ret;
+		}
+	}
+
 	if (priv->tft > max_fifo_threshold) {
 		dev_warn(dev, "requested TX FIFO threshold %u, adjusted down to %u\n",
 			 priv->tft, max_fifo_threshold);

-- 
2.44.0

Re: [PATCH v2 09/11] i2c: nomadik: support Mobileye EyeQ5 I2C controller

[Linus Walleij]

On Thu, Feb 29, 2024 at 7:10 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> Add compatible for the integration of the same DB8500 IP block into the
> Mobileye EyeQ5 platform. Two quirks are present:
>
>  - The memory bus only supports 32-bit accesses. Avoid writeb() and
>    readb() by introducing helper functions that fallback to writel()
>    and readl().
>
>  - A register must be configured for the I2C speed mode; it is located
>    in a shared register region called OLB. We access that memory region
>    using a syscon & regmap that gets passed as a phandle (mobileye,olb).
>
>    A two-bit enum per controller is written into the register; that
>    requires us to know the global index of the I2C controller (cell arg
>    to the mobileye,olb phandle).
>
> We add #include <linux/mfd/syscon.h> and <linux/regmap.h> and sort
> headers.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 09/11] i2c: nomadik: support Mobileye EyeQ5 I2C controller

[Wolfram Sang]

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


> +	if (of_device_is_compatible(np, "mobileye,eyeq5-i2c")) {
> +		ret = nmk_i2c_eyeq5_probe(priv);
> +		if (ret) {
> +			dev_info(dev, "%s: %d: %d\n", __func__, __LINE__, ret);

This is debug code, or? Please remove it. Especially since
nmk_i2c_eyeq5_probe() prints something out on error.


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

Re: [PATCH v2 09/11] i2c: nomadik: support Mobileye EyeQ5 I2C controller

[Théo Lebrun]

Hello,

On Mon Mar 4, 2024 at 10:27 AM CET, Wolfram Sang wrote:
>
> > +	if (of_device_is_compatible(np, "mobileye,eyeq5-i2c")) {
> > +		ret = nmk_i2c_eyeq5_probe(priv);
> > +		if (ret) {
> > +			dev_info(dev, "%s: %d: %d\n", __func__, __LINE__, ret);
>
> This is debug code, or? Please remove it. Especially since
> nmk_i2c_eyeq5_probe() prints something out on error.

It is. Nice catch, sorry about that.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 09/11] i2c: nomadik: support Mobileye EyeQ5 I2C controller

[Andi Shyti]

Hi Theo,

...

> +#include <linux/amba/bus.h>
>  #include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
>  #include <linux/init.h>
> -#include <linux/module.h>
> -#include <linux/amba/bus.h>
> -#include <linux/slab.h>
>  #include <linux/interrupt.h>
> -#include <linux/i2c.h>
> -#include <linux/err.h>
> -#include <linux/clk.h>
>  #include <linux/io.h>
> -#include <linux/pm_runtime.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>

Please reorder the headers in a different patch.

>  #define DRIVER_NAME "nmk-i2c"
>  

...

> +static inline u8 nmk_i2c_readb(const struct nmk_i2c_dev *priv,
> +			       unsigned long reg)
> +{
> +	if (priv->has_32b_bus)
> +		return readl(priv->virtbase + reg);
> +	else
> +		return readb(priv->virtbase + reg);

nit: no need for the else (your choice though, if you want to
have ti coherent with the write counterpart).

> +}
> +
> +static inline void nmk_i2c_writeb(const struct nmk_i2c_dev *priv, u32 val,
> +				unsigned long reg)
> +{
> +	if (priv->has_32b_bus)
> +		writel(val, priv->virtbase + reg);
> +	else
> +		writeb(val, priv->virtbase + reg);
> +}

...

> +static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
> +{
> +	struct device *dev = &priv->adev->dev;
> +	struct device_node *np = dev->of_node;
> +	unsigned int shift, speed_mode;
> +	struct regmap *olb;
> +	unsigned int id;
> +
> +	priv->has_32b_bus = true;
> +
> +	olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id);
> +	if (IS_ERR_OR_NULL(olb)) {
> +		if (!olb)
> +			olb = ERR_PTR(-ENOENT);
> +		return dev_err_probe(dev, PTR_ERR(olb),
> +				     "failed OLB lookup: %lu\n", PTR_ERR(olb));

just return PTR_ERR(olb) and use dev_err_probe() in the upper
layer probe.

> +	}
> +
> +	if (priv->clk_freq <= 400000)
> +		speed_mode = 0b00;
> +	else if (priv->clk_freq <= 1000000)
> +		speed_mode = 0b01;
> +	else
> +		speed_mode = 0b10;

would be nice to have these as defines.

> +
> +	shift = NMK_I2C_EYEQ5_OLB_IOCR2_SHIFT(id);
> +	regmap_update_bits(olb, NMK_I2C_EYEQ5_OLB_IOCR2,
> +			   0b11 << shift, speed_mode << shift);

please define these values and for hexadecimals use 0x...

> +	return 0;
> +}
> +
>  static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
>  {
>  	int ret = 0;
> @@ -1001,8 +1065,17 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
>  
>  	priv->vendor = vendor;
>  	priv->adev = adev;
> +	priv->has_32b_bus = false;
>  	nmk_i2c_of_probe(np, priv);
>  
> +	if (of_device_is_compatible(np, "mobileye,eyeq5-i2c")) {
> +		ret = nmk_i2c_eyeq5_probe(priv);
> +		if (ret) {
> +			dev_info(dev, "%s: %d: %d\n", __func__, __LINE__, ret);
> +			return ret;
> +		}

as said above, use dev_err_probe here.

Andi

> +	}
> +
>  	if (priv->tft > max_fifo_threshold) {
>  		dev_warn(dev, "requested TX FIFO threshold %u, adjusted down to %u\n",
>  			 priv->tft, max_fifo_threshold);
> 
> -- 
> 2.44.0
> 

Re: [PATCH v2 09/11] i2c: nomadik: support Mobileye EyeQ5 I2C controller

[Théo Lebrun]

Hello,

On Mon Mar 4, 2024 at 3:08 PM CET, Andi Shyti wrote:
> Hi Theo,
>
> ...
>
> > +#include <linux/amba/bus.h>
> >  #include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> >  #include <linux/init.h>
> > -#include <linux/module.h>
> > -#include <linux/amba/bus.h>
> > -#include <linux/slab.h>
> >  #include <linux/interrupt.h>
> > -#include <linux/i2c.h>
> > -#include <linux/err.h>
> > -#include <linux/clk.h>
> >  #include <linux/io.h>
> > -#include <linux/pm_runtime.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/pinctrl/consumer.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
>
> Please reorder the headers in a different patch.

Will do.

>
> >  #define DRIVER_NAME "nmk-i2c"
> >  
>
> ...
>
> > +static inline u8 nmk_i2c_readb(const struct nmk_i2c_dev *priv,
> > +			       unsigned long reg)
> > +{
> > +	if (priv->has_32b_bus)
> > +		return readl(priv->virtbase + reg);
> > +	else
> > +		return readb(priv->virtbase + reg);
>
> nit: no need for the else (your choice though, if you want to
> have ti coherent with the write counterpart).

Indeed, the useless else block can be removed. Will do.

> > +}
> > +
> > +static inline void nmk_i2c_writeb(const struct nmk_i2c_dev *priv, u32 val,
> > +				unsigned long reg)
> > +{
> > +	if (priv->has_32b_bus)
> > +		writel(val, priv->virtbase + reg);
> > +	else
> > +		writeb(val, priv->virtbase + reg);
> > +}
>
> ...
>
> > +static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
> > +{
> > +	struct device *dev = &priv->adev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	unsigned int shift, speed_mode;
> > +	struct regmap *olb;
> > +	unsigned int id;
> > +
> > +	priv->has_32b_bus = true;
> > +
> > +	olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id);
> > +	if (IS_ERR_OR_NULL(olb)) {
> > +		if (!olb)
> > +			olb = ERR_PTR(-ENOENT);
> > +		return dev_err_probe(dev, PTR_ERR(olb),
> > +				     "failed OLB lookup: %lu\n", PTR_ERR(olb));
>
> just return PTR_ERR(olb) and use dev_err_probe() in the upper
> layer probe.

Good catch. In previous revisions nmk_i2c_eyeq5_probe() had multiple
error cases so it had to be the one doing the logging. Now that there
is a single possible error parent can do it. It should simplify code.

>
> > +	}
> > +
> > +	if (priv->clk_freq <= 400000)
> > +		speed_mode = 0b00;
> > +	else if (priv->clk_freq <= 1000000)
> > +		speed_mode = 0b01;
> > +	else
> > +		speed_mode = 0b10;
>
> would be nice to have these as defines.

Agreed. Will be named based on I2C mode names, eg standard, fast, high
speed, fast plus.

>
> > +
> > +	shift = NMK_I2C_EYEQ5_OLB_IOCR2_SHIFT(id);
> > +	regmap_update_bits(olb, NMK_I2C_EYEQ5_OLB_IOCR2,
> > +			   0b11 << shift, speed_mode << shift);
>
> please define these values and for hexadecimals use 0x...

0b11 is a two-bit mask. What do you mean by "these values"? Something
like:



#define NMK_I2C_EYEQ5_SPEED_MODE_FAST		0b00
#define NMK_I2C_EYEQ5_SPEED_MODE_FAST_PLUS	0b01
#define NMK_I2C_EYEQ5_SPEED_MODE_HIGH_SPEED	0b10

static const u8 nmk_i2c_eyeq5_masks[] = {
	[0] = 0x0030,
	[1] = 0x00C0,
	[2] = 0x0300,
	[3] = 0x0C00,
	[4] = 0x3000,
};

static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
{
	// ...
	unsigned int id, mask, speed_mode;

	priv->has_32b_bus = true;

	olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id);
	// TODO: olb error checking
	// TODO: check id is valid

	if (priv->clk_freq <= 400000)
		speed_mode = NMK_I2C_EYEQ5_SPEED_MODE_FAST;
	else if (priv->clk_freq <= 1000000)
		speed_mode = NMK_I2C_EYEQ5_SPEED_MODE_FAST_PLUS;
	else
		speed_mode = NMK_I2C_EYEQ5_SPEED_MODE_HIGH_SPEED;

	mask = nmk_i2c_eyeq5_masks[id];
	regmap_update_bits(olb, NMK_I2C_EYEQ5_OLB_IOCR2,
			   mask, speed_mode << __fls(mask));
	return 0;
}

Else I do not see what you are suggesting by "please define these
values".

Have a nice day,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v2 10/11] MIPS: mobileye: eyeq5: add 5 I2C controller nodes

[Théo Lebrun]

Add the SoC I2C controller nodes to the platform devicetree. Use a
default bus frequency of 400kHz. They are AMBA devices that are matched
on PeriphID.

Set transfer timeout to 10ms instead of Linux's default of 200ms.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 arch/mips/boot/dts/mobileye/eyeq5.dtsi | 75 ++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
index 8d4f65ec912d..540d55503f3b 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
+++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
@@ -70,6 +70,81 @@ soc: soc {
 		ranges;
 		compatible = "simple-bus";
 
+		i2c0: i2c@300000 {
+			compatible = "mobileye,eyeq5-i2c", "arm,primecell";
+			reg = <0 0x300000 0x0 0x1000>;
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SHARED 1 IRQ_TYPE_LEVEL_HIGH>;
+			clock-frequency = <400000>; /* Fast mode */
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&i2c_ser_clk>, <&i2c_clk>;
+			clock-names = "i2cclk", "apb_pclk";
+			resets = <&reset 0 13>;
+			i2c-transfer-timeout-us = <10000>;
+			mobileye,olb = <&olb 0>;
+		};
+
+		i2c1: i2c@400000 {
+			compatible = "mobileye,eyeq5-i2c", "arm,primecell";
+			reg = <0 0x400000 0x0 0x1000>;
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SHARED 2 IRQ_TYPE_LEVEL_HIGH>;
+			clock-frequency = <400000>; /* Fast mode */
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&i2c_ser_clk>, <&i2c_clk>;
+			clock-names = "i2cclk", "apb_pclk";
+			resets = <&reset 0 14>;
+			i2c-transfer-timeout-us = <10000>;
+			mobileye,olb = <&olb 1>;
+		};
+
+		i2c2: i2c@500000 {
+			compatible = "mobileye,eyeq5-i2c", "arm,primecell";
+			reg = <0 0x500000 0x0 0x1000>;
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SHARED 3 IRQ_TYPE_LEVEL_HIGH>;
+			clock-frequency = <400000>; /* Fast mode */
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&i2c_ser_clk>, <&i2c_clk>;
+			clock-names = "i2cclk", "apb_pclk";
+			resets = <&reset 0 15>;
+			i2c-transfer-timeout-us = <10000>;
+			mobileye,olb = <&olb 2>;
+		};
+
+		i2c3: i2c@600000 {
+			compatible = "mobileye,eyeq5-i2c", "arm,primecell";
+			reg = <0 0x600000 0x0 0x1000>;
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SHARED 4 IRQ_TYPE_LEVEL_HIGH>;
+			clock-frequency = <400000>; /* Fast mode */
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&i2c_ser_clk>, <&i2c_clk>;
+			clock-names = "i2cclk", "apb_pclk";
+			resets = <&reset 0 16>;
+			i2c-transfer-timeout-us = <10000>;
+			mobileye,olb = <&olb 3>;
+		};
+
+		i2c4: i2c@700000 {
+			compatible = "mobileye,eyeq5-i2c", "arm,primecell";
+			reg = <0 0x700000 0x0 0x1000>;
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SHARED 5 IRQ_TYPE_LEVEL_HIGH>;
+			clock-frequency = <400000>; /* Fast mode */
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&i2c_ser_clk>, <&i2c_clk>;
+			clock-names = "i2cclk", "apb_pclk";
+			resets = <&reset 0 17>;
+			i2c-transfer-timeout-us = <10000>;
+			mobileye,olb = <&olb 4>;
+		};
+
 		uart0: serial@800000 {
 			compatible = "arm,pl011", "arm,primecell";
 			reg = <0 0x800000 0x0 0x1000>;

-- 
2.44.0

Re: [PATCH v2 10/11] MIPS: mobileye: eyeq5: add 5 I2C controller nodes

[Linus Walleij]

On Thu, Feb 29, 2024 at 7:11 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> Add the SoC I2C controller nodes to the platform devicetree. Use a
> default bus frequency of 400kHz. They are AMBA devices that are matched
> on PeriphID.
>
> Set transfer timeout to 10ms instead of Linux's default of 200ms.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v2 11/11] MIPS: mobileye: eyeq5: add evaluation board I2C temp sensor

[Théo Lebrun]

Declare the temperature sensor on I2C bus 2. Its label is the schematics
identifier.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 arch/mips/boot/dts/mobileye/eyeq5-epm5.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts b/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts
index 6898b2d8267d..9fc1a1b0a81b 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts
+++ b/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts
@@ -21,3 +21,11 @@ memory@0 {
 		      <0x8 0x02000000 0x0 0x7E000000>;
 	};
 };
+
+&i2c2 {
+	temperature-sensor@48 {
+		compatible = "ti,tmp112";
+		reg = <0x48>;
+		label = "U60";
+	};
+};

-- 
2.44.0

Re: [PATCH v2 11/11] MIPS: mobileye: eyeq5: add evaluation board I2C temp sensor

[Linus Walleij]

On Thu, Feb 29, 2024 at 7:11 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> Declare the temperature sensor on I2C bus 2. Its label is the schematics
> identifier.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 00/11] Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts

[Andi Shyti]

Hi Theo,

> Théo Lebrun (11):
>       dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example
>       dt-bindings: hwmon: lm75: use common hwmon schema
>       i2c: nomadik: rename private struct pointers from dev to priv
>       i2c: nomadik: simplify IRQ masking logic
>       i2c: nomadik: use bitops helpers
>       i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer
>       i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout
>       i2c: nomadik: fetch i2c-transfer-timeout-us property from devicetree
>       i2c: nomadik: support Mobileye EyeQ5 I2C controller
>       MIPS: mobileye: eyeq5: add 5 I2C controller nodes
>       MIPS: mobileye: eyeq5: add evaluation board I2C temp sensor

what's your plan for this series? If you extract into a separate
series the refactoring patches that are not dependent on the
bindings I could queue them up for the merge window.

Andi

Re: [PATCH v2 00/11] Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts

[Théo Lebrun]

Hello Andi,

On Wed Mar 6, 2024 at 2:49 AM CET, Andi Shyti wrote:
> > Théo Lebrun (11):
> >       dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example
> >       dt-bindings: hwmon: lm75: use common hwmon schema
> >       i2c: nomadik: rename private struct pointers from dev to priv
> >       i2c: nomadik: simplify IRQ masking logic
> >       i2c: nomadik: use bitops helpers
> >       i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer
> >       i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout
> >       i2c: nomadik: fetch i2c-transfer-timeout-us property from devicetree
> >       i2c: nomadik: support Mobileye EyeQ5 I2C controller
> >       MIPS: mobileye: eyeq5: add 5 I2C controller nodes
> >       MIPS: mobileye: eyeq5: add evaluation board I2C temp sensor
>
> what's your plan for this series? If you extract into a separate
> series the refactoring patches that are not dependent on the
> bindings I could queue them up for the merge window.

V3 is ready and will be sent today. I think we can get trailers from
dt-bindings maintainers as the discussion has been caried out on this
revision.

Am I being too optimistic of seeing this series queued before the merge
window?

Thanks Andi,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com