On Mon, 8 May 2023 15:03:28 +0200
Philippe De Muyter <Philippe.DeMuyter@xxxxxxx> 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@xxxxxxx> wrote:
> > >
> > > > From: Philippe De Muyter <phdm@xxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxx>
> > > > Cc: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > > > Cc: Michael Hennerich <Michael.Hennerich@xxxxxxxxxx>
> > > > Cc: Jonathan Cameron <jic23@xxxxxxxxxx>
> > > > ---
> > > > 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
