Message-ID: <CAM+2EuJDzjNY7+epBbNVqrOoY60SYFsoyX6uXEAf9hyu-kaKQA@mail.gmail.com>
Date:   Mon, 31 Jan 2022 12:59:55 +0530
From:   jagath jogj <jagathjog1996@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        lars@...afoo.de, aardelean@...iqon.com, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: IIO Device Driver for Maxim DS3502 potentiometer

On Sun, Jan 30, 2022 at 6:31 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Sat, 29 Jan 2022 10:24:16 +0530
> jagath jogj <jagathjog1996@...il.com> wrote:
>
> > Hello Jonathan and Andy Shevchenko,
> >
> > Thanks for replying.
> >
> > On Fri, Jan 28, 2022 at 8:14 PM Andy Shevchenko
> > <andriy.shevchenko@...ux.intel.com> wrote:
> > >
> > > On Fri, Jan 28, 2022 at 10:35:54AM +0000, Jonathan Cameron wrote:
> > > > On Fri, 28 Jan 2022 09:11:28 +0530
> > > > jagath jogj <jagathjog1996@...il.com> wrote:
> > > >
> > > > > Hello,
> > > > >
> > > > > I have a Maxim DS3502 potentiometer breakout and I have written an IIO
> > > > > driver for learning purposes and tested with Raspberry pi and wanted
> > > > > to send patches of the driver for the IIO sub-system.
> > > > >
> > > > > Can I send the patches for DS3502 POT for review?
> > > > >
> > > > > The setup used to write driver
> > > > > Raspberry pi 3b
> > > > > DS3502 breakout board
> > > > > Raspberry pi latest kernel branch - https://github.com/raspberrypi/linux
> > >
> > > > Welcome to IIO.
> > > >
> > > > Absolutely on sending the patches for review.
> > > > You'll need to rebase them on latest mainline from kernel.org
> > > > (pick a tagged version which would currently be 5.17-rc1_
> >
> > I am using raspberry pi kernel branch rpi-5.17-y which is based on
> > mainline tag 5.17-rc1.
> > Is it required to rebase the changes to the latest tag version
> > 5.17-rc1 from kernel.org?
>
> I'll probably be fine as I wouldn't expect the raspberry pi tree to
> be carrying any changes in this area.  If there are minor issues I can
> usually just fix them up whilst applying.

Thank you

>
> >
> > > >
> > > > and then follow the documentation for how to submit a patch in
> > > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> >
> > Sure I will follow the documentation for submitting a patch.
> > I am also learning and recently submitted a patch series of code-style
> > fixes to the staging branch.
> >
> > > >
> > > > Feel free to ask if you have any questions about the process.
> > > >
> > > > Looking forwards to seeing your code.
> > >
> > > Agree with Jonathan.
> > >
> > > One remark though, can you double check that drivers/iio/potentiometer
> > > doesn't have any similar driver that can be expanded (usually it can be
> > > done by analyzing a register file of the devices, like register offsets
> > > and their meanings or bit fields)?
>
> Excellent question.
>
> >
> > In iio/potentiometer folder the existing Maxim DS1803 is having some
> > differences with DS3502 like
> >
> > Maxim DS1803:
> > Number of wipers - 2
> > Number of Positions - 256 - 8 bit.
> > Memory map having 2 volatile registers used to store wiper value.
> > https://datasheets.maximintegrated.com/en/ds/DS1803.pdf
> >
> >
> > Maxim DS3502:
> > Number of wipers - 1
> > Number of Positions - 128 - 7 bit.
> > The memory map has 2 registers to store wiper value and mode
> > Supports non-volatile memory to store wiper value
> > Supports 2 modes - Mode 0 and Mode 1
> > https://datasheets.maximintegrated.com/en/ds/DS3502.pdf
> >
> >
> > So thought of writing the driver for DS3502 in a separate file.
> > Need some advice on this whether to implement it on a separate file or
> > to extend the existing driver.
>
> These potentiometer drivers tend to be very simple, (the ds1803 is
> only 167 lines long so you may find that you'd end up adding more
> code to make it flexible enough to take your new part than a
> new driver would need.
>
> So perhaps the question we should ask is if we are likely to see
> support for a wide range of similar parts?  If we are then now
> is a good time to make the driver more flexible.
>
> Working that out will require some datasheet diving..
> My guess is the ds3501 is pretty similar but with some extra bits.

Yeah DS3501 is similar to DS3502 with an additional temperature sensor.

>
> Given these are fairly simple, your best route to an answer might
> be to try adding it to the existing driver and see if you run
> into any significant complexity.
>
> It's not unheard of for us to merge drivers together in the future
> once it becomes clear that we are supporting lots of similar ones
> but it is easier done at the start!
>
> Jonathan

Thanks for your input. I will try to add the DS3502 with the existing
driver DS1803
then I will try to add DS3501 to the same driver.
There are drivers like iio/temperature/maxim_thermocouple.c and
iio/adc/max1027.c
where multiple devices with some differences are handled with a single
driver file.
I will consider them as references to add the ds3502 into existing ds1803.c.


>
>
> >
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
> > >
> >
> > Regards,
> > Jagath
>

Regards,
Jagath

Powered by blists - more mailing lists