From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Subject: Re: [PATCH 1/1] of: introduce helper to manage boolean Date: Tue, 10 Jul 2012 14:10:44 +0200 Message-ID: References: <1328588028-24766-1-git-send-email-plagnioj@jcrosoft.com> <20120309014441.8236B3E0903@localhost> <20120309100435.GN27213@game.jcrosoft.org> <20120309162638.92E813E0880@localhost> <20120309163608.GQ27213@game.jcrosoft.org> <4F5E50DA.6080000@freescale.com> <20120313031739.GA18320@game.jcrosoft.org> <20120313041652.563B63E0536@localhost> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5266629284117132324==" Return-path: In-Reply-To: <20120313041652.563B63E0536@localhost> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org Sender: "devicetree-discuss" To: Grant Likely Cc: Scott Wood , devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, Rob Herring , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: devicetree@vger.kernel.org --===============5266629284117132324== Content-Type: multipart/alternative; boundary=e89a8fb20532bb17a104c4789e90 --e89a8fb20532bb17a104c4789e90 Content-Type: text/plain; charset=ISO-8859-1 Hi, On Tue, Mar 13, 2012 at 5:16 AM, Grant Likely wrote: > On Tue, 13 Mar 2012 04:17:39 +0100, Jean-Christophe PLAGNIOL-VILLARD < > plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote: > > On 14:39 Mon 12 Mar , Scott Wood wrote: > > > On 03/09/2012 10:36 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: > > > >>>> Ugh. so any value other than 1 returns false? I think that will > surprise > > > >>>> most people. > > > >>>> > > > >>>> I don't like this api or binding. If it is a bool property, then > why isn't > > > >>>> simply testing for the property existance sufficient? > > > >>> no if you want to disable it > > > >>> > > > >>> if a bool is define in the dtsi and want to disable it int the dts > > > >>> > > > >>> if you we can do the the invert > > > >>> > > > >>> if !0 => true > > > >>> > > > >>> is-ok; => true > > > >>> is-ok = ; => true > > > >>> is-ok = <0>; => false > > > >> > > > >> This is a failure of the dtc tool, not the binding. Accepting this > binding > > > >> means we have to live with it for a very long time. It needs to be > fixed > > > >> in dtc instead so that properties can be deleted instead of only > modified. > > > > I understand your idea but today if you put and value in the > property it's true. > > > > > > > > So is-ok = <0>; is true also which is illogical as in any language a > boolean is > > > > true (1) or false (0). When I read the property I will understand > false not true > > > > > > You could say similar things about is-ok = "no" or is-ok = "" or is-ok > = > > > "I'd rather you didn't"... it's expected that violating the binding may > > > produce illogical results. > > today is most of the binding people use a number whe the want to be able > to > > delete it and it's the same in most of the promgramming language > > It isn't yet a big pain point, so there isn't time pressure here. Fixing > the tool > is the better solution, and since the kernel carries a copy of the tool we > don't > need to worry about adding a feature that isn't available by the dtc > packaged by > a distribution. > > Fixing the tool is the correct thing to do. > I just wanted to pick up on this thread. Now in the kernel, absence of a property indicates false, and presence indicates true. This is a kernel convention, nothing to do with the dtb format. I think it is useful to support a boolean with a non-null value which can be 0, meaning true as Jean-Christophe says. My reasons are: 1. dtc does not have a way to delete a property and it isn't clear what syntax could be used there. It also seems a little brittle to delete a property in one place and define it in another (ordering? confusion when people can't work out why it has gone?). So not only can dtc not do this, but I'm not sure that it should. 2. fdtput allows updating a property. It is pretty easy to do something like 'fdtput file.dtb /node propname $value' where value is 0 or 1. To use the current binding we need to either do: if [[ $value == 0 ]]; then fdtput -d file.dtb /node propname # new function to delete a property? else fdtput file.dtb /node propname 1 fi or perhaps something like: fdtput -B file.dtb /node propname ${value} where -B is a new option meaning either create an empty property, or delete any property it finds, based on the value being 0 or non-zero. 3. Discoverability: it is nice to be able to see the possible options, even if disabled. In particular, if a boolean value is true, and you later decide with fdtput to turn it off, you effectively remove all trace of it. This could be confusing for those who are looking for available options. Arguably these issues could be fixed if we introduced some sort of type hinting into the dtb file format, but that seems a long shot. I am not sure that modifying the tools to support this arguable odd 'present/absent' boolean binding is the right approach. So I think Jean-Christophe's idea has significant merit. Thoughts? Regards, Simon > > g. > > _______________________________________________ > devicetree-discuss mailing list > devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org > https://lists.ozlabs.org/listinfo/devicetree-discuss > --e89a8fb20532bb17a104c4789e90 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Hi,

On Tue, Mar 13, 2012 at 5:16 AM, Gran= t Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
On Tue, 13 Mar 2012 04:17:39 +0100,= Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:
> On 14:39 Mon 12 Mar =A0 =A0 , Scott Wood wrote:
> > On 03/09/2012 10:36 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: > > >>>> Ugh. =A0so any value other than 1 returns false?= =A0I think that will surprise
> > >>>> most people.
> > >>>>
> > >>>> I don't like this api or binding. =A0If it i= s a bool property, then why isn't
> > >>>> simply testing for the property existance suffic= ient?
> > >>> no if you want to disable it
> > >>>
> > >>> if a bool is define in the dtsi and want to disable = it int the dts
> > >>>
> > >>> if you we can do the the invert
> > >>>
> > >>> if !0 =3D> true
> > >>>
> > >>> is-ok; =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=3D> tr= ue
> > >>> is-ok =3D <val !=3D 0>; =A0 =A0 =3D> true > > >>> is-ok =3D <0>; =A0 =A0 =A0 =A0 =A0 =A0=3D> = false
> > >>
> > >> This is a failure of the dtc tool, not the binding. =A0A= ccepting this binding
> > >> means we have to live with it for a very long time. =A0I= t needs to be fixed
> > >> in dtc instead so that properties can be deleted instead= of only modified.
> > > I understand your idea but today if you put and value in the= property it's true.
> > >
> > > So is-ok =3D <0>; is true also which is illogical as i= n any language a boolean is
> > > true (1) or false (0). When I read the property I will under= stand false not true
> >
> > You could say similar things about is-ok =3D "no" or is= -ok =3D "" or is-ok =3D
> > "I'd rather you didn't"... it's expected th= at violating the binding may
> > produce illogical results.
> today is most of the binding people use a number whe the want to be ab= le to
> delete it and it's the same in most of the promgramming language
It isn't yet a big pain point, so there isn't time pres= sure here. =A0Fixing the tool
is the better solution, and since the kernel carries a copy of the tool we = don't
need to worry about adding a feature that isn't available by the dtc pa= ckaged by
a distribution.

Fixing the tool is the correct thing to do.

=
I just wanted to pick up on this thread. Now in the kernel, absence of= a property indicates false, and presence indicates true. This is a kernel = convention, nothing to do with the dtb format. I think it is useful to supp= ort a boolean with a non-null value which can be 0, meaning true as Jean-Ch= ristophe says.

My reasons are:

1. dtc does no= t have a way to delete a property and it isn't clear what syntax could = be used there. It also seems a little brittle to delete a property in one p= lace and define it in another (ordering? confusion when people can't wo= rk out why it has gone?). So not only can dtc not do this, but I'm not = sure that it should.

2. fdtput allows updating a property. It is pretty easy= to do something like 'fdtput file.dtb /node propname $value' where= value is 0 or 1. To use the current binding we need to either do:

if [[ $value =3D=3D 0 ]]; then
=A0 =A0fdtput = -d file.dtb /node=A0propname =A0 # new function to delete a property?
=
else
=A0 =A0fdtput file.dtb /node=A0propname 1=A0
= fi

or perhaps something like:

=A0 =A0= fdtput -B file.dtb /node=A0propname ${value} =A0=A0

where -B is a new option meaning either create an empty property, or dele= te any property it finds, based on the value being 0 or non-zero.

3. Discoverability: it is nice to be able to see = the possible options, even if disabled. In particular, if a boolean value i= s true, and you later decide with fdtput to turn it off, you effectively re= move all trace of it. This could be confusing for those who are looking for= available options.

Arguably these issues could be fixed if we introduced s= ome sort of type hinting into the dtb file format, but that seems a long sh= ot. I am not sure that modifying the tools to support this arguable odd = 9;present/absent' boolean binding is the right approach.

So I think=A0Jean-Christophe's idea has significant= merit.

Thoughts?

Regards= ,
Simon=A0

g.

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@l= ists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

--e89a8fb20532bb17a104c4789e90-- --===============5266629284117132324== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ devicetree-discuss mailing list devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org https://lists.ozlabs.org/listinfo/devicetree-discuss --===============5266629284117132324==--