Re: [PATCH] iio: dac: ad5446: Add ID of compatible Texas Instruments i2c dac121c*

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 8 May 2023 15:03:28 +0200
Philippe De Muyter <Philippe.DeMuyter@macq.eu> wrote:

> Hello Jonathan,
> 
> I have discovered that linux v6.0 already supports the ti,dac121c081
> chips in the ti-dac5571.c driver.  There's thus no need for my patch,
> that I had written because I work with a much older kernel.
> 
> Sorry for the noise.

No problem. Great that you found this out!

> 
> Maybe the different dac drivers supporting similar chips should be
> merged, but that's anoter story.

Definitely sounds like it's worth looking at.

Jonathan


> 
> Best regards
> 
> Philippe
> 
> On Mon, May 08, 2023 at 10:37:19AM +0200, Philippe De Muyter wrote:
> > Hello Jonathan,
> > 
> > On Sun, May 07, 2023 at 02:46:08PM +0100, Jonathan Cameron wrote:  
> > > On Sun,  7 May 2023 11:10:25 +0200
> > > Philippe De Muyter <Philippe.DeMuyter@macq.eu> wrote:
> > >   
> > > > From: Philippe De Muyter <phdm@macqel.be>
> > > > 
> > > > The Texas Instruments DAC121C* chips are the I2C counterparts of
> > > > the DAC121S* SPI chips which are already supported by this ad5446 driver.
> > > > 
> > > > Add them to the compatible list.  
> > > 
> > > Hi Philippe,
> > > 
> > > DT binding should be updated and include the fallback to adi,ad5622.
> > > Does this driver actually have a bindings doc?  If not please add one
> > > as a precursor patch then add binding for this new part on top.  
> > 
> > No, there's no ad5446 to find in Documentation/ :(
> > 
> > I will try to make one.
> >   
> > >   
> > > > 
> > > > Signed-off-by: Philippe De Muyter <phdm@macqel.be>
> > > > Cc: Lars-Peter Clausen <lars@metafoo.de>
> > > > Cc: Michael Hennerich <Michael.Hennerich@analog.com>
> > > > Cc: Jonathan Cameron <jic23@kernel.org>
> > > > ---
> > > >  drivers/iio/dac/ad5446.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/drivers/iio/dac/ad5446.c b/drivers/iio/dac/ad5446.c
> > > > index aa3130b33456..b95c0ccbb796 100644
> > > > --- a/drivers/iio/dac/ad5446.c
> > > > +++ b/drivers/iio/dac/ad5446.c
> > > > @@ -587,6 +587,7 @@ static const struct i2c_device_id ad5446_i2c_ids[] = {
> > > >  	{"ad5602", ID_AD5602},
> > > >  	{"ad5612", ID_AD5612},
> > > >  	{"ad5622", ID_AD5622},
> > > > +	{"dac121", ID_AD5622}, /* 'ti,' is dropped by 'of_modalias_node' */  
> > > 
> > > True, but why is the comment needed?  
> > 
> > I will remove it
> >   
> > > Also, for consistency with the spi equivalent it should be dac121c101 or similar
> > > I think.  
> > 
> > Actually the chip I use is a dac121c085, and there exists also dac121c081.
> > They share the same datasheet.  The difference is only in the number of
> > i2c addresses the hardware designer may choose from and the pin used for the
> > external Vref.
> > 
> > Do you prefer I add only dac121c085, both dac121c085 and dac121c081, or
> > stick to the common part of the name, with or without the 'c' that stands
> > for i2C, like the 's' in dac121s101 stands for Spi ?
> > 
> > The documentation also mentions lower-resolution chips in the same family :
> > dac101c081 and dac101c085 (10 bit resolution), and dac081c081 and dac081c085
> > (8 bit resolution).
> >   
> > > 
> > > I think this use of the driver with multiple vendor prefixes,
> > > also indicates we should really add the of_device_id table for this
> > > driver. To do that nicely will require more changes as we'd want to do
> > > the same for the SPI side which has a single entry table (which is odd)
> > > then deal with the data fields which should probably all be pointers
> > > rather than enum values.
> > > 
> > > Still I'm fine with proper explicit DT support being left for a follow up patch.
> > > 
> > > I do want the missing binding doc fixed though (which is independent of the
> > > question of how the driver binds based on the compatible values).
> > > 
> > > Thanks,
> > > 
> > > Jonathan
> > > 
> > >   
> > > >  	{}
> > > >  };
> > > >  MODULE_DEVICE_TABLE(i2c, ad5446_i2c_ids);  
> > 
> > Best regards
> > 
> > Philippe