Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 22 Jun 2020 16:10:54 +0100
From:   Lee Jones <lee.jones@linaro.org>
To:     Frank Rowand <frowand.list@gmail.com>
Cc:     andy.shevchenko@gmail.com, michael@walle.cc, robh+dt@kernel.org,
        broonie@kernel.org, devicetree@vger.kernel.org,
        linus.walleij@linaro.org, linux@roeck-us.net,
        andriy.shevchenko@linux.intel.com, robin.murphy@arm.com,
        gregkh@linuxfoundation.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match
 devices with the correct of_nodes
Message-ID: <20200622151054.GW954398@dell>
References: <20200611191002.2256570-1-lee.jones@linaro.org>
 <4b188fb5-6667-720d-46e1-6f103efe8966@gmail.com>
 <20200615092644.GA2608702@dell>
 <eef50a78-8571-5600-4fee-c824fd4a7f69@gmail.com>
 <20200622085009.GP954398@dell>
 <cd8952da-cc55-8087-b9f6-876417beb188@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <cd8952da-cc55-8087-b9f6-876417beb188@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 22 Jun 2020, Frank Rowand wrote:

> On 2020-06-22 03:50, Lee Jones wrote:
> > On Thu, 18 Jun 2020, Frank Rowand wrote:
> > 
> >> On 2020-06-15 04:26, Lee Jones wrote:
> >>> On Sun, 14 Jun 2020, Frank Rowand wrote:
> >>>
> >>>> Hi Lee,
> >>>>
> >>>> I'm looking at 5.8-rc1.
> >>>>
> >>>> The only use of OF_MFD_CELL() where the same compatible is specified
> >>>> for multiple elements of a struct mfd_cell array is for compatible
> >>>> "stericsson,ab8500-pwm" in drivers/mfd/ab8500-core.c:
> >>>>
> >>>>         OF_MFD_CELL("ab8500-pwm",
> >>>>                     NULL, NULL, 0, 1, "stericsson,ab8500-pwm"),
> >>>>         OF_MFD_CELL("ab8500-pwm",
> >>>>                     NULL, NULL, 0, 2, "stericsson,ab8500-pwm"),
> >>>>         OF_MFD_CELL("ab8500-pwm",
> >>>>                     NULL, NULL, 0, 3, "stericsson,ab8500-pwm"),
> >>
> >>          OF_MFD_CELL("ab8500-pwm",
> >>                      NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
> >>
> >>          OF_MFD_CELL_REG("ab8500-pwm-mc",
> >>                          NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
> >>          OF_MFD_CELL_REG("ab8500-pwm-mc",
> >>                          NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
> >>          OF_MFD_CELL_REG("ab8500-pwm-mc",
> >>                          NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
> >>
> >>>>
> >>>> The only .dts or .dtsi files where I see compatible "stericsson,ab8500-pwm"
> >>>> are:
> >>>>
> >>>>    arch/arm/boot/dts/ste-ab8500.dtsi
> >>>>    arch/arm/boot/dts/ste-ab8505.dtsi
> >>>>
> >>>> These two .dtsi files only have a single node with this compatible.
> >>>> Chasing back to .dts and .dtsi files that include these two .dtsi
> >>>> files, I see no case where there are multiple nodes with this
> >>>> compatible.
> >>>>
> >>>> So it looks to me like there is no .dts in mainline that is providing
> >>>> the three "stericsson,ab8500-pwm" nodes that drivers/mfd/ab8500-core.c
> >>>> is expecting.  No case that there are multiple mfd child nodes where
> >>>> mfd_add_device() would assign the first of n child nodes with the
> >>>> same compatible to multiple devices.
> >>>>
> >>>> So it appears to me that drivers/mfd/ab8500-core.c is currently broken.
> >>>> Am I missing something here?
> >>>>
> >>>> If I am correct, then either drivers/mfd/ab8500-core.c or
> >>>> ste-ab8500.dtsi and ste-ab8505.dtsi need to be fixed.
> >>>
> >>> Your analysis is correct.
> >>
> >> OK, if I'm not overlooking anything, that is good news.
> >>
> >> Existing .dts source files only have one "ab8500-pwm" child.  They already
> >> work correcly.
> >>
> >> Create a new compatible for the case of multiple children.  In my example
> >> I will add "-mc" (multiple children) to the existing compatible.  There
> >> is likely a better name, but this lets me provide an example.
> >>
> >> Modify drivers/mfd/ab8500-core.c to use the new compatible, and new .dts
> >> source files with multiple children use the new compatible:
> >>
> >>          OF_MFD_CELL("ab8500-pwm",
> >>                      NULL, NULL, 0, 0, "stericsson,ab8500-pwm"),
> >>
> >>          OF_MFD_CELL_REG("ab8500-pwm-mc",
> >>                          NULL, NULL, 0, 0, "stericsson,ab8500-pwm", 0),
> >>          OF_MFD_CELL_REG("ab8500-pwm-mc",
> >>                          NULL, NULL, 0, 1, "stericsson,ab8500-pwm", 1),
> >>          OF_MFD_CELL_REG("ab8500-pwm-mc",
> >>                          NULL, NULL, 0, 2, "stericsson,ab8500-pwm", 2),
> >>
> >> The "OF_MFD_CELL" entry is the existing entry, which will handle current
> >> .dts source files.  The new "OF_MFD_CELL_REG" entries will handle new
> >> .dts source files.
> > 
> > Sorry, but I'm not sure what the above exercise is supposed to solve.
> > 
> > Could you explain it for me please?
> 
> The OF_MFD_CELL() entry handles all of the existing .dts source files
> that only have one ab8500-pwm child nodes.  So existing .dtb blobs
> continue to work.
> 
> The OF_MFD_CELL_REG() entries will handle all of the new .dts source
> files that will have up to 3 ab8500-pwm child nodes.
> 
> Compatibility is maintained for existing .dtb files.  A new kernel
> version with the changes will support new .dtb files that contain
> multiple ab8500-pwm child nodes.

I can see *what* you're trying to do.  I was looking for an
explanation of *how* you think that will work.  FWIW, I don't think
what you're proposing will work as you envisage.  I thought that
perhaps I was missing something, which is why I requested further
explanation.

> >> And of course the patch that creates OF_MFD_CELL_REG() needs to precede
> >> this change.
> >>
> >> I would remove the fallback code in the existing patch that tries to
> >> handle an incorrect binding.  Just error out if the binding is not
> >> used properly.
> > 
> > What fallback code?
> 
> Based on reading the patch description, I expected some extra code to try
> to handle the case where the compatible in more than one struct mfd_cell
> entry is "stericsson,ab8500-pwm" and there are multiple ab8500-pwm child
> nodes.
> 
> Looking at the actual code (which I had not done before), I see that the
> "best effort attempt to match" is keeping a list of child nodes that
> have already been used (mfd_of_node_list) and avoiding re-use of such
> nodes.  This allows an invalid .dtb (one with multple "stericsson,ab8500-pwm"
> child nodes) to possibly be assigned unique child nodes for multiple
> struct mfd_cell entries to be "stericsson,ab8500-pwm".
> 
> So it is confusing for me to call that "fallback code".  It really is
> "best effort attempt to match" for a broken .dtb code.
> 
> There should be no best effort for a broken .dtb.  The broken .dtb should
> instead result in an error.

The problem is, how can you tell the difference between a valid and a
broken FDT without pre-processing - which, as I explained in the
commit message, I am not prepared to do.  We cannot test individually
since all configurations (e.g. no 'reg' property are valid on an
individual basis.

The best we can do is "best effort", to try and match each cell with
its requested OF node.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog