> On Oct 31, 2016, at 4:39 AM, Vignesh R <vigne...@ti.com> wrote: > > > > On Friday 28 October 2016 02:47 AM, John Syne wrote: > >>>>>>>>>>> >>>>>>>>>>> --- >>>>>>>>>>> include/linux/mfd/ti_am335x_tscadc.h | 4 ++-- >>>>>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>>>>>>> >>>>>>>>>>> diff --git a/include/linux/mfd/ti_am335x_tscadc.h >>>>>>>>>>> b/include/linux/mfd/ti_am335x_tscadc.h >>>>>>>>>>> index b9a53e0..96c4207 100644 >>>>>>>>>>> --- a/include/linux/mfd/ti_am335x_tscadc.h >>>>>>>>>>> +++ b/include/linux/mfd/ti_am335x_tscadc.h >>>>>>>>>>> @@ -90,7 +90,7 @@ >>>>>>>>>>> /* Delay register */ >>>>>>>>>>> #define STEPDELAY_OPEN_MASK (0x3FFFF << 0) >>>>>>>>>>> #define STEPDELAY_OPEN(val) ((val) << 0) >>>>>>>>>>> -#define STEPCONFIG_OPENDLY STEPDELAY_OPEN(0x098) >>>>>>>>> Wouldn’t this be better to add this to the devicetree? >>>>>>>>> >>>>>>>>> ti,chan-step-avg = <0x16 0x16 0x16 0x16 0x16 0x16 0x16>; >>>>>>>>> ti,chan-step-opendelay = <0x500 0x500 0x500 0x500 0x500 0x500 >>>>>>>>> 0x500>; >>>>>>>>> ti,chan-step-sampledelay = <0x0 0x0 0x0 0x0 0x0 0x0 0x0>; >>>>>>>> >>>>>>>> For a touch screen, there is not need to change in these parameter >>>>>>>> settings, so my opinion is to keep it as is. Or am I missing something? >>>>>>> I was thinking that if you are using this driver as an ADC, you may >>>>>>> want the flexibility to make these changes in the DT. I’m doing this by >>>>>>> connecting sensors to the ADC inputs. I’m not using this driver for a >>>>>>> touchscreen. >>>>>> >>>>>> Here is a DT overlay were this gets using on the BeagleBoneBlack. >>>>>> >>>>>> https://github.com/RobertCNelson/bb.org-overlays/blob/master/src/arm/BB-ADC-00A0.dts >>>>>> >>>>>> Besides, these DT features are already implemented in the driver so it >>>>>> is just a matter of adding these entries to the am33xx.dtsi & >>>>>> am4372.dtsi, which you modified in this patch series. >>>>> >>>>> This looks like configuration, no? >>>>> >>>>> DT should be used to describe the hardware. >>>> You may be right, but how is this different to setting the baud rate on a >>>> serial channel or sampling rate on a audio channel? Looking through the >>>> DT, there are many configuration settings, so I’m not sure what is the >>>> correct way to handle this. Surely it is better to handle this in DT vs >>>> hard coding these settings? >>> >>> I think setting the UART baud rate is also an invalid DT entry. >>> >>> It's okay to list all of the options in DT, but to actually select >>> one, that should be done either in userspace or as a kernel option. >>> Perhaps as a Kconfig selection. >> Yeah, this has been inconsistent for a long time. My only point was that >> these DT parameters had already been implemented in the ti_am335x_adc KM and >> I thought that this was better than hard coding these settings. Implementing >> this in Kconfig means rebuilding the KM, which isn’t desirable. > >> Perhaps this should be done via sysfs attributes so as you say, a userspace >> app can configure this driver. > > This was discussed when DT properties were added. Patches are welcome > to add sysfs entries. There is nothing wrong with specifying an initial > value in the DT. > >> I guess the DT code in ti_am335x_adc.c should be removed. >> > > Removing DT properties is not an option as it will break DT backward > compatibility. Hi Vignesh,
OK, then back to my original question. Given that these DT properties are supported in the driver, shouldn’t the following be added to am33xx.dtsi and am4372.dtsi? ti,chan-step-avg = <0x16 0x16 0x16 0x16 0x16 0x16 0x16>; ti,chan-step-opendelay = <0x500 0x500 0x500 0x500 0x500 0x500 0x500>; ti,chan-step-sampledelay = <0x0 0x0 0x0 0x0 0x0 0x0 0x0>; Regards, John > > -- > Regards > Vignesh
