lkml.org 
[lkml]   [2012]   [Jan]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] x86/platform: (TS-5500) revised ADC driver
    Hi Guenter,

    On Fri, 20 Jan 2012 20:46:31 -0800,
    Guenter Roeck <guenter.roeck@ericsson.com> wrote:

    > On Fri, Jan 20, 2012 at 06:43:22PM -0500, Vivien Didelot wrote:
    > > Update of the ADC driver according to the Guenter Roeck's review.
    > >
    > > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
    > > ---
    > > arch/x86/platform/ts5500/ts5500_adc.c | 228
    > > +++++++++++++-------------------- 1 files changed, 89
    > > insertions(+), 139 deletions(-)
    > >
    > > diff --git a/arch/x86/platform/ts5500/ts5500_adc.c
    > > b/arch/x86/platform/ts5500/ts5500_adc.c index fc4560f..da6decf
    > > 100644 --- a/arch/x86/platform/ts5500/ts5500_adc.c
    > > +++ b/arch/x86/platform/ts5500/ts5500_adc.c
    > > @@ -62,57 +62,62 @@
    > >
    > Hi Vivien,
    >
    > This isn't really a revised driver ... it is a patch on top of the
    > previous version.

    True, sorry for the mistake.

    > Regarding the location, I'd really like to know from the
    > powers-that-be if arch/x86/platform/ts5500/
    > or
    > drivers/platform/x86
    > or
    > drivers/hwmon
    >
    > would be the appropriate location for a driver like this. As
    > mentioned before, my strong preference is drivers/hwmon, but I would
    > like to hear from others.

    We should either split every driver into corresponding subdirectories,
    or put everything in a common platform directory. My first RFC patches
    set has every driver separated. As they are really specific to the
    platform, people seem to agree with grouping them, mainly because they
    won't be shared. I changed that in the following patches sets, and X86
    maintainers seemed to be ok with that.

    I'm ok with both solutions, but we should all agree on one.
    Maybe we should have other maintainers view on this?

    > Also, I am not sure if the current approach is appropriate to start
    > with. Looking at the datasheet as well as into existing kernel code,
    > it appears quite likely that some kind of more or less generic MAX197
    > driver exists somewhere. The existence of is_max197_installed() -
    > without any calling code - is a strong indication that this is the
    > case, as well as the "static" platform data in your original patch.
    > It might be more appropriate to take this more or less generic
    > driver, move it to drivers/hwmon, and provide - for example through
    > platform data - a means to read from and write to the chip on a
    > per-platform basis, ie with per-platform access functions.

    You're right, it should be possible to create a generic max197 driver
    and provide read/write functions through platform data. But we don't
    have a max197 right now... So it can stay as a compact TS-5500 ADC
    driver for the moment, and maybe we will split later. What do you think?

    Thanks,

    -Vivien


    \
     
     \ /
      Last update: 2012-01-23 02:39    [W:0.023 / U:61.184 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site