linux-kernel - Re: [PATCH 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 09 Sep 2014 12:32:26 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Stanimir Varbanov <svarbanov@...sol.com>
Cc:	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Pawel Moll <pawel.moll@....com>,
	Rob Herring <robh+dt@...nel.org>,
	Kumar Gala <galak@...eaurora.org>,
	Mark Rutland <mark.rutland@....com>,
	Grant Likely <grant.likely@...aro.org>,
	Jonathan Cameron <jic23@...nel.org>,
	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Hartmut Knaack <knaack.h@....de>,
	Angelo Compagnucci <angelo.compagnucci@...il.com>,
	Doug Anderson <dianders@...omium.org>,
	Fugang Duan <B38611@...escale.com>,
	Johannes Thumshirn <johannes.thumshirn@....de>,
	Jean Delvare <jdelvare@...e.de>,
	Philippe Reynes <tremyfr@...oo.fr>,
	Lee Jones <lee.jones@...aro.org>,
	Josh Cartwright <joshc@...eaurora.org>,
	Stephen Boyd <sboyd@...eaurora.org>,
	David Collins <collinsd@...eaurora.org>,
	"Ivan T. Ivanov" <iivanov@...sol.com>
Subject: Re: [PATCH 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver

On Monday 08 September 2014 18:30:00 Stanimir Varbanov wrote:
> >>> These numbers all look hardware specific, so why put macros into the
> >>> device tree rather than using them directly?
> >>
> >> The idea was to use #defines in DT nodes when we need to overwrite the
> >> adc channel parameters, see example in 2/2 how it will be used.
> > 
> > I don't understand. The node in the example has
> > 
> > +               /* Channel node */
> > +               usb_id_nopull@39 {
> > +                       qcom,channel = <VADC_LR_MUX10_USB_ID>;
> > ...
> > +               };
> > 
> > 
> > And VADC_LR_MUX10_USB_ID is defined to 0x39.  How is this helping anything?
> > You just introduce an artificial dependency on the header file, which makes
> > it a mess to merge the patches or do updates, and anybody who needs to
> > make updates to this now has to go through the same pain, to update the
> > dts files, the driver and the binding document in lockstep.
> > 
> > Why not remove the qcom,channel property completely and use a 'reg'
> > property with #address-cells=<1>, #size-cells=<0> and put the number
> > directly in there, with no need for obfuscation macros?
> 
> OK thanks for the remarks. I will fix this mess.
> 
> I hope you are expecting to see this:
> 
> pmic_vadc: vadc@...0 {
>         #address-cells = <1>;
>         #size-cells = <0>;
>         #io-channel-cells = <1>;
>         io-channel-ranges;
> 
>         usb_id_nopull@39 {
>                 reg = <0x39>;
>         };
> };
> 
> and use the vadc channel from usb device node
> 
> usb {
>         ...
>         io-channels = <&pmic_vadc 0x39>;
>         io-channel-names = "usbidnopull";
> };

The ID stuff looks good now, but I had not noticed the
"io-channel-names" property before. I think you misunderstood
the purpose of that, because it is very similar to the name of the
adc provider (usb_id_nopull@39).

Like anything else that we refer to by name (interrupt, reg,
clock, regulator, ...), the name used in the client is supposed
to be a string that identifies what the connection means to the
client, not what it means to the provider. This string is
supposed to be defined in the binding of the client device and
independent of what other hardware block provides it.

E.g. if you have two usb devices that need a separate adc channel,
it could look like

pmic_vadc: vadc@...0 {

	usb_1_id_nopull@39 {
		reg = 0x39;
	};
	usb_2_id_nopull@40 {
		reg = 0x40;
	};

};

usb@...000 {
	io-channels = <&pmic_vadc 0x39>;
	io-channel-names = "vadc";
};

usb@...000 {
	io-channels = <&pmic_vadc 0x40>;
	io-channel-names = "vadc";
};

Now the usb driver just asks for an io channel named "vadc"
and the ADC core code will perform that lookup.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/