Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 18 Feb 2022 12:24:20 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Jagath Jog J <jagathjog1996@gmail.com>
Cc:     Andy Shevchenko <andy.shevchenko@gmail.com>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Slawomir Stepien <sst@poczta.fm>,
        Rob Herring <robh+dt@kernel.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 3/4] iio: potentiometer: Add support for Maxim DS3502
Message-ID: <20220218122420.76425450@jic23-huawei>
In-Reply-To: <20220214195252.GA7374@jagath-PC>
References: <20220214033620.4059-1-jagathjog1996@gmail.com>
        <20220214033620.4059-4-jagathjog1996@gmail.com>
        <CAHp75VcWym5vyDAVyTUCpj=Qkm28VUaqdqJ7VuFL_bsb0fmhaA@mail.gmail.com>
        <20220214195252.GA7374@jagath-PC>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 15 Feb 2022 01:22:54 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> Hello Andy,
> 
> On Mon, Feb 14, 2022 at 01:32:14PM +0200, Andy Shevchenko wrote:
> > On Mon, Feb 14, 2022 at 5:36 AM Jagath Jog J <jagathjog1996@gmail.com> wrote:  
> > >
> > > The DS3502 is a 7-bit, nonvolatile digital potentiometer featuring
> > > an output voltage range of up to 15.5V.
> > > DS3502 support is implemented into existing ds1803 driver  
> > 
> > Be consistent here and in other commit messages with how you refer to
> > the IC parts, i.e.
> > DS1803. Don't forget English grammar and punctuation, i.e. missed period above.
> >   
> 
> I will fix this in v3
> 
> > > Datasheet: https://datasheets.maximintegrated.com/en/ds/DS3502.pdf  
> >   
> > >  
> > 
> > A tag block may not have blank lines. Drop it.
> >   
> > > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>  
> > 
> > ...
> >   
> > > -       tristate "Maxim Integrated DS1803 Digital Potentiometer driver"
> > > +       tristate "Maxim Integrated DS1803 and similar Digital Potentiometer driver"  
> > 
> > Please, list them like other drivers do:
> > 
> >        tristate "Maxim Integrated DS1803/DS... Digital Potentiometer driver"
> > 
> > ...
> >   
> > > -         Say yes here to build support for the Maxim Integrated DS1803
> > > -         digital potentiometer chip.
> > > +         Say yes here to build support for the Maxim Integrated DS1803 and
> > > +         similar digital potentiometer chip.  
> > 
> > Same here.

Usual thing is to use the and similar phrasing for the title if it
is getting to long and have the one place that lists them all being the belp
text.  We need it somewhere so people can grep for it.

> > 
> > ...
> >   
> > > - * Maxim Integrated DS1803 digital potentiometer driver
> > > + * Maxim Integrated DS1803 and similar digital potentiometer driver  
> > 
> > Same here.  
> 
> Based on Jonathan suggestion for the previous patch version I used 
> "and similar" wording here.

I wasn't that clear on exactly where to do that!  Sorry about that.

> 
> > 
> > ...
> >   
> > > -#define DS1803_MAX_POS         255
> > > -#define DS1803_WRITE(chan)     (0xa8 | ((chan) + 1))  
> > 
> > Not sure why these were removed (or moved?)  
> 
> Since max wiper position is present in avail array of ds1803_cfg structure
> and that is being used for read scale so DS1803_MAX_POS is removed.
> 
> Since each wiper address of both parts is assigned to the address
> member of iio_chan_spec struct so DS1803_WRITE(chan) is removed.
> 
> > 
> > ...
> >   
> > > +static const struct ds1803_cfg ds1803_cfg[] = {
> > > +       [DS1803_010] = { .wipers = 2, .avail = { 0, 1, 255 }, .kohms =  10,
> > > +                        .channels = ds1803_channels,
> > > +                        .num_channels = ARRAY_SIZE(ds1803_channels) },
> > > +       [DS1803_050] = { .wipers = 2, .avail = { 0, 1, 255 }, .kohms =  50,
> > > +                        .channels = ds1803_channels,
> > > +                        .num_channels = ARRAY_SIZE(ds1803_channels) },
> > > +       [DS1803_100] = { .wipers = 2, .avail = { 0, 1, 255 }, .kohms = 100,
> > > +                        .channels = ds1803_channels,
> > > +                        .num_channels = ARRAY_SIZE(ds1803_channels) },
> > > +       [DS3502] =     { .wipers = 1, .avail = { 0, 1, 127 }, .kohms =  10,
> > > +                        .channels = ds3502_channels,
> > > +                        .num_channels = ARRAY_SIZE(ds3502_channels) },
> > >  };  
> > 
> > Split this on a per type basis. I believe it won't be too much work,
> > also, consider adding channels as a separate preparatory patch as you
> > did with avail.  
> 
> Based on Jonathan suggestion for the previous patch version to avoid
> having different chip type related structures so channels and num_channels
> are added into ds1803_cfg structure.
> 
> Sure for channels I will split into separate patch for old part in v3.

I'm not totally sure what Andy is suggesting with his feedback here.

> 
> >
> > ...
> >   
> > > -       data->cfg = &ds1803_cfg[id->driver_data];
> > > +       data->chip_type = (uintptr_t)device_get_match_data(dev);
> > > +       if (data->chip_type < DS1803_010 || data->chip_type > DS3502)
> > > +               data->chip_type = id->driver_data;  
> > 
> > Split it into a separate patch and use pointer validation instead:
> > 
> > data->cfg = ...
> > if (!data->cfg)
> >   data->cfg = ...id->driver_data;
> > 
> > ...
> >   
> > > -       { .compatible = "maxim,ds1803-010", .data = &ds1803_cfg[DS1803_010] },
> > > -       { .compatible = "maxim,ds1803-050", .data = &ds1803_cfg[DS1803_050] },
> > > -       { .compatible = "maxim,ds1803-100", .data = &ds1803_cfg[DS1803_100] },  
> 
> To get the chip specific structure I can use previous structure method for data
> and validation as you shown above.
> But it is necessary to get the chip_type also because of dependency in 
> ds1803_raw_read().

You could use a function pointer in the cfg structure and provide appropriate read
functions for the different types so that read_raw() can call the appropriate function
for the type of device.  That would be tidy and avoid this need to change
to an enum.

Sorry for slow response btw as I didn't get in before v3!

Thanks,

Jonathan


> 
> To get the chip_type can I use 
> data->chip_type = id->driver_data
> 
> > > +       { .compatible = "maxim,ds1803-010", .data = (void *)DS1803_010 },
> > > +       { .compatible = "maxim,ds1803-050", .data = (void *)DS1803_050 },
> > > +       { .compatible = "maxim,ds1803-100", .data = (void *)DS1803_100 },  
> > 
> > This is not good, please use pointers as it was before.
> >   
> > > +       { .compatible = "maxim,ds3502",     .data = (void *)DS3502 },  
> > 
> > Ditto. Create a new, separate structure for this type.
> > 
> > ...
> >   
> > >         { "ds1803-010", DS1803_010 },
> > >         { "ds1803-050", DS1803_050 },
> > >         { "ds1803-100", DS1803_100 },
> > > +       { "ds3502",     DS3502     },  
> > 
> > Too many spaces.
> > Besides this, please create a new prerequisite patch to convert this
> > to use pointers as above.  
> 
> Sure I will split this patch in v3.
> Thanks for feedback.
> 
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko  
> 
> Best Regards,
> Jagath