| 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
| ||
|
Message-ID: <20131024172734.GB2461@kartoffel>
Date: Thu, 24 Oct 2013 18:27:35 +0100
From: Mark Rutland <mark.rutland@....com>
To: Manish Badarkhe <badarkhe.manish@...il.com>
Cc: "devicetree-discuss@...ts.ozlabs.org"
<devicetree-discuss@...ts.ozlabs.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"davinci-linux-open-source@...ux.davincidsp.com"
<davinci-linux-open-source@...ux.davincidsp.com>,
"grant.likely@...retlab.ca" <grant.likely@...retlab.ca>,
"dmitry.torokhov@...il.com" <dmitry.torokhov@...il.com>,
"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
"rob@...dley.net" <rob@...dley.net>
Subject: Re: [PATCH V4 1/2] tps6507x-ts: Add DT support
On Thu, Oct 24, 2013 at 06:05:53PM +0100, Manish Badarkhe wrote:
> Hi Mark,
>
> Thank you for your reply.
>
> On Wed, Oct 23, 2013 at 10:15 PM, Mark Rutland <mark.rutland@....com> wrote:
>
> On Wed, Oct 23, 2013 at 05:28:52PM +0100, Manish Badarkhe wrote:
> > Add device tree based support for TI's tps6507x touchscreen.
> >
> > Signed-off-by: Manish Badarkhe <badarkhe.manish@...il.com>
> > ---
> > Changes since V3:
> > - Rebased on top of Dmitry's changes
> > - Removed error handling for optional DT properties
> >
> > Changes since V2:
> > - Removed unnecessary code.
> > - Updated Documentation to provide proper names of
> > devicetree properties.
> >
> > Changes since V1:
> > - Updated documentation to specify tps6507x as multifunctional
> > device.
> > - return proper error value in absence of platform and DT
> > data for touchscreen.
> > - Updated commit message.
> >
> > :100755 100755 8fffa3c... e1b9cfd... M Documentation/devicetree/
> bindings/mfd/tps6507x.txt
> > :100644 100644 db604e0... 0cf0eb1... M drivers/input/touchscreen/
> tps6507x-ts.c
> > Documentation/devicetree/bindings/mfd/tps6507x.txt | 24 ++++++-
> > drivers/input/touchscreen/tps6507x-ts.c | 72
> ++++++++++++--------
> > 2 files changed, 67 insertions(+), 29 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/
> Documentation/devicetree/bindings/mfd/tps6507x.txt
> > index 8fffa3c..e1b9cfd 100755
> > --- a/Documentation/devicetree/bindings/mfd/tps6507x.txt
> > +++ b/Documentation/devicetree/bindings/mfd/tps6507x.txt
> > @@ -1,4 +1,8 @@
> > -TPS6507x Power Management Integrated Circuit
> > +TPS6507x Multifunctional Device.
> > +
> > +Features provided by TPS6507x:
> > + 1.Power Management Integrated Circuit.
> > + 2.Touch-Screen.
> >
> > Required properties:
> > - compatible: "ti,tps6507x"
> > @@ -23,6 +27,9 @@ Required properties:
> > vindcdc1_2-supply: VDCDC1 and VDCDC2 input.
> > vindcdc3-supply : VDCDC3 input.
> > vldo1_2-supply : VLDO1 and VLDO2 input.
> > +- tsc: This node specifies touch screen data.
>
> This is a node, but is listed un "Required properties". That seems odd.
>
> When is it required?
>
> Why is the data under the tsc subnode? Why not directly under the main
> node?
>
>
> Yes, It seems odd to add a subnode properties here. I gone through other
> documentation
> for MFD devices and observed that separate documentation file is present for
> sub node modules.
I was trying to say that nodes != properties rather than that this should be
split into separate files.
>
> TPS6507x is multifunctional device and having touch screen submodule. As it is
> child node for
> TPS6507x multifunctional device, I have created child node as "tsc".
>
>
>
> > + ti,poll-period : Time at which touch input is getting sampled in
> ms.
>
> Is this for debounce? Other bindings have similar but differently named
> properties.
>
>
> This is not debounce time. This time is required for input poll devices.
>
>
> Why is this necessary? Can the driver not choose a sane value?
>
>
> poll-period is necessary to sample touch inputs. Driver will chose value
> default poll
> period if this value is not present. I was bit confused here, whether to add
> this property
> as optional or required. As with default period touch not achieve performance.
> Can I make this property optional?
Please elaborate. Why will the default period be sub-optimal? What's the
tradeoff here?
>
>
> > + ti,min-pressure: Minimum pressure value to trigger touch.
>
> What type are these? Single (u32) cells?
>
> What units is ti,min-pressure in?
>
>
> No, its a u16 cell. It is measured in ohm. Again default value for min-pressure
> is available
> in driver code. Can I make this property optional?
Why is it a u16, it's very uncommon to have u16 properties.
What _physical_ units is this in, and what order of magnitude? e.g. Pascals,
millipascals.
>
>
> >
> > Regulator Optional properties:
> > - defdcdc_default: It's property of DCDC2 and DCDC3 regulators.
> > @@ -30,6 +37,14 @@ Regulator Optional properties:
> > 1: If defdcdc pin of DCDC2/DCDC3 is driven HIGH.
> > If this property is not defined, it defaults to 0 (not enabled).
> >
> > +Touchscreen Optional properties:
> > +- ti,vendor : Touchscreen vendor id to populate
> > + in sysclass interface.
> > +- ti,product: Touchscreen product id to populate
> > + in sysclass interface.
> > +- ti,version: Touchscreen version id to populate
> > + in sysclass interface.
>
> Are these integers, strings, or something else?
>
>
> These are unsigned short(u16) values.
Why?
>
>
> What values make sense?
>
>
> These values are only to tell about chip details.
That does not describe the set of valid values.
Is ti,vendor = <4> valid? What does this mean?
Is there some standard for assignment of vendor IDs that this follows?
>
>
> What's the sysclass interface? That sounds like a Linux detail. The
> properties
> might be fine but the description shouldn't need to refer to Linux
> internals.
>
>
> Okay, I will update description for these properties.
>
>
> > +
> > Example:
> >
> > pmu: tps6507x@48 {
> > @@ -88,4 +103,11 @@ Example:
> > };
> > };
> >
> > + tsc {
> > + ti,poll-period = <30>;
> > + ti,min-pressure = <0x30>;
> > + ti,vendor = <0>;
> > + ti,product = <65070>;
> > + ti,version = <0x100>;
> > + };
> > };
> > diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/
> touchscreen/tps6507x-ts.c
> > index db604e0..0cf0eb1 100644
> > --- a/drivers/input/touchscreen/tps6507x-ts.c
> > +++ b/drivers/input/touchscreen/tps6507x-ts.c
> > @@ -22,6 +22,8 @@
> > #include <linux/mfd/tps6507x.h>
> > #include <linux/input/tps6507x-ts.h>
> > #include <linux/delay.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> >
> > #define TSC_DEFAULT_POLL_PERIOD 30 /* ms */
> > #define TPS_DEFAULT_MIN_PRESSURE 0x30
> > @@ -206,33 +208,54 @@ done:
> > tps6507x_adc_standby(tsc);
> > }
> >
> > +static void tsc_init_data(struct tps6507x_ts *tsc,
> > + struct input_dev *input_dev)
> > +{
> > + struct device_node *node = tsc->dev->of_node;
> > + const struct tps6507x_board *tps_board =
> > + dev_get_platdata(tsc->dev);
> > + const struct touchscreen_init_data *init_data = NULL;
> > +
> > + if (node)
> > + node = of_find_node_by_name(node, "tsc");
>
> As of_find_node_by_name traverses the list of all nodes (not just
> children),
> this may match nodes other than what you expect.
>
>
> I think, It traverse nodes from parent node (i.e. tps6507x) and find child.
> Please correct me if I am wrong?
No. As I said, it traverses the list of all nodes. Is there is no matching
child, it will go on and possibly match a node that is not a direct child (e.g.
a child of another node).
Perhaps of_get_child_by_name is what you want.
>
>
>
> > + if (tps_board)
> > + /*
> > + * init_data points to array of touchscreen_init structure
> > + * coming from the board-evm file.
> > + */
> > + init_data = tps_board->tps6507x_ts_init_data;
> > +
> > + if (node == NULL || init_data == NULL) {
> > + tsc->poll_dev->poll_interval = TSC_DEFAULT_POLL_PERIOD;
> > + tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
> > + } else if (init_data) {
> > + tsc->poll_dev->poll_interval = init_data->poll_period;
> > + tsc->min_pressure = init_data->min_pressure;
> > + input_dev->id.vendor = init_data->vendor;
> > + input_dev->id.product = init_data->product;
> > + input_dev->id.version = init_data->version;
> > + } else if (node) {
> > + of_property_read_u32(node, "ti,poll-period",
> > + &tsc->poll_dev->poll_interval);
> > + of_property_read_u16(node, "ti,min-pressure",
> > + &tsc->min_pressure);
>
> You didn't mention these were 16-bit values in the binding.
>
> As DTB is encoded big-endian, and you didn't use a /bits/ 16 declaration
> (making the property a u32 cell), won't this read 0 in all cases you have a
> value in the expected range (as in your example)?
>
>
> I will mention these values as 16bit. values in binding.
Please explain _why_ they are 16-bit values. Even if they are 16-bit it may
make sense to have them as u32 values for general consistency and least
surprise.
Thanks,
Mark.
--
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/
Powered by blists - more mailing lists