IIO Device Driver for Maxim DS3502 potentiometer

IIO Device Driver for Maxim DS3502 potentiometer

[jagath jogj]

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

Regards,
Jagath

Re: IIO Device Driver for Maxim DS3502 potentiometer

[Jonathan Cameron]

On Fri, 28 Jan 2022 09:11:28 +0530
jagath jogj <jagathjog1996@gmail.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
> 
> Regards,
> Jagath

Hi Jagath,

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_

and then follow the documentation for how to submit a patch in
https://www.kernel.org/doc/html/latest/process/submitting-patches.html

Feel free to ask if you have any questions about the process.

Looking forwards to seeing your code.

Thanks,

Jonathan

Re: IIO Device Driver for Maxim DS3502 potentiometer

[Andy Shevchenko]

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@gmail.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_
> 
> and then follow the documentation for how to submit a patch in
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> 
> 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)?

-- 
With Best Regards,
Andy Shevchenko

Re: IIO Device Driver for Maxim DS3502 potentiometer

[jagath jogj]

Hello Jonathan and Andy Shevchenko,

Thanks for replying.

On Fri, Jan 28, 2022 at 8:14 PM Andy Shevchenko
<andriy.shevchenko@linux.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@gmail.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?

> >
> > 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)?

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.

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

Regards,
Jagath

Re: IIO Device Driver for Maxim DS3502 potentiometer

[Jonathan Cameron]

On Sat, 29 Jan 2022 10:24:16 +0530
jagath jogj <jagathjog1996@gmail.com> wrote:

> Hello Jonathan and Andy Shevchenko,
> 
> Thanks for replying.
> 
> On Fri, Jan 28, 2022 at 8:14 PM Andy Shevchenko
> <andriy.shevchenko@linux.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@gmail.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.

> 
> > >
> > > 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.

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


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

Re: IIO Device Driver for Maxim DS3502 potentiometer

[jagath jogj]

On Sun, Jan 30, 2022 at 6:31 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 29 Jan 2022 10:24:16 +0530
> jagath jogj <jagathjog1996@gmail.com> wrote:
>
> > Hello Jonathan and Andy Shevchenko,
> >
> > Thanks for replying.
> >
> > On Fri, Jan 28, 2022 at 8:14 PM Andy Shevchenko
> > <andriy.shevchenko@linux.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@gmail.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