Re: [PATCH 2/2] dt-bindings: mfd: ahc1ec0.yaml: Add Advantech Embedded Controll - AHC1EC0

[Rob Herring]

On Fri, Oct 23, 2020 at 1:30 AM Shihlun.Lin
<Shihlun.Lin@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Rob,
>
> Thank you for taking the time for checking.
>
> > -----Original Message-----
> > From: Rob Herring [mailto:robh@xxxxxxxxxx]
> > Sent: Saturday, October 17, 2020 2:31 AM
> > To: Shihlun.Lin
> > Cc: Lee Jones; linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > Campion.Kang; AceLan Kao
> > Subject: Re: [PATCH 2/2] dt-bindings: mfd: ahc1ec0.yaml: Add Advantech
> > Embedded Controll - AHC1EC0
> >
> > On Wed, Oct 14, 2020 at 04:29:41PM +0800, Shihlun Lin wrote:
> > > Add DT binding schema for Advantech embedded controller AHC1EC0.
> >
> > Where's the driver?
> >
>
> Because the maintainers are different people, I separated the submission
> into three groups: maintainer, device tree, and Kernel driver. Should I put
> all maintainers into one Email and send all patches in a group at once?
>
> > >
> > > Signed-off-by: Shihlun Lin <shihlun.lin@xxxxxxxxxxxxxxxx>
> > > ---
> > >  .../devicetree/bindings/mfd/ahc1ec0.yaml      | 76
> > +++++++++++++++++++
> > >  1 file changed, 76 insertions(+)
> > >  create mode 100644
> > Documentation/devicetree/bindings/mfd/ahc1ec0.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/mfd/ahc1ec0.yaml
> > b/Documentation/devicetree/bindings/mfd/ahc1ec0.yaml
> > > new file mode 100644
> > > index 000000000000..2a3581d2ecab
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mfd/ahc1ec0.yaml
> > > @@ -0,0 +1,76 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +# All the top-level keys are standard json-schema keywords except for
> > > +# 'maintainers' and 'select'
> >
> > Drop this.
> >
>
> I checked other yaml files. Should I drop all content above, or just remove
> the last two lines?
>
> > > +
> > > +
> > > +$id: http://devicetree.org/schemas/mfd/ahc1ec0.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Advantech Embedded Controller (AHC1EC0)
> > > +
> > > +maintainers:
> > > +  - Shihlun Lin <shihlun.lin@xxxxxxxxxxxxxxxx>
> > > +  - Campion Kang <campion.kang@xxxxxxxxxxxxxxxx>
> > > +
> > > +description: |
> > > +  AHC1EC0 is one of the embedded controllers used by Advantech to
> > provide several
> > > +  functions such as watchdog, hwmon, brightness, etc. Advantech
> > related applications
> > > +  can control the whole system via these functions.
> > > +
> > > +  # In this case, a 'false' schema will never match.
> >
> > Drop
> >
>
> I checked other yaml files. I should remove the last line only, right?
>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: advantech,ahc1ec0
> > > +
> > > +  advantech,sub-dev-nb:
> > > +    description:
> > > +      The number of sub-devices specified in the platform.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    maxItems: 1
> >
> > Isn't this just the length of the next property?
> >
>
> You are right, exactly. This is just the number of item about the next property.
> This property will be used in the driver later.
>
> > > +
> > > +  advantech,sub-dev:
> > > +    description:
> > > +      A list of the sub-devices supported in the platform. Defines for
> > the
> > > +      appropriate values can found in dt-bindings/mfd/ahc1ec0.h.
> > > +    $ref: "/schemas/types.yaml#/definitions/uint32-array"
> > > +    minItems: 1
> > > +    maxItems: 6
> >
> > This is kind of odd. Aren't you going to need DT info for each of the
> > sub-dev. GPIO is a provider, backlight connection, LED properties, etc.
> >
>
> We will submit 6 sub-devices step-by-step totally. But this time, we only submit
> 2 sub-devices to upstream (Watchdog and HWMonitor).
>
> Should we remove all information about other sub-devices in the
> mfd core Kernel driver, and device tree documentation? Or just keep them?

You should submit a binding for a complete device. At least, as
complete as possible.

What you have here isn't something any other MFD device has done. So
that's not the right way as there's not likely anything special about
this device.

Rob