On Sun, Sep 10, 2017 at 04:11:08PM +0100, Jonathan Cameron wrote:
> On Tue, 5 Sep 2017 11:27:00 +0200 Lukas Wunner <lukas@xxxxxxxxx> wrote:
> > The DACrrcS085 (rr = 08/10/12, c = 2/4) family of SPI DACs was
> > inherited by TI when they acquired National Semiconductor in 2011.
>
> Very nice. A few really minor comments inline.
D'accord on all your comments except this one:
> > + ret = sscanf(spi->modalias, "dac%2hhu%1d",
> > + &ti_dac->resolution, &indio_dev->num_channels);
> > + WARN_ON(ret != 2);
>
> Whilst this seems nice and clear now, it may not work if this driver
> had additional parts added in future.
>
> I would prefer an explicit table with this information in it and
> use an enum to reference into it.
>
> This is a bit 'too clever' :)
Hm, I looked at the other SPI DACs available from TI and their programming
interface is sufficiently different that they don't lend themselves all
too well for integration into this driver. There might be DACs from other
vendors with a similar programming interface.
What I *could* do is encode the resolution and number of channels in the
driver_data field of the spi MODULE_DEVICE_TABLE like this:
static const struct spi_device_id ti_dac_spi_id[] = {
{ "dac082s085", 8 << 8 + 2 },
{ "dac102s085", 10 << 8 + 2 },
...
Then instead of reading the information from the modalias I'd read them
from driver_data. However it looks a bit redundant and silly to me.
Can we add this extra complexity when the need for it actually arises?
Thanks,
Lukas
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
