Re: [PATCH] iio: max5481: Add support for Maxim digital potentiometers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Peter,

On Wed, Dec 7, 2016 at 1:04 PM, Peter Meerwald-Stadler
<pmeerw@xxxxxxxxxx> wrote:
>
>> Add implementation for Maxim Integrated 5481, 5482, 5483,
>> and 5484 digital potentiometer devices.
>
> nitpicking below

Thanks for the review.  At this time we aren't in a position to spend
time on updates but just wanted to share what we pulled together for
others to start from.  I'll mark this as rejected.

Thanks!

>
>> Datasheet:
>> http://datasheets.maximintegrated.com/en/ds/MAX5481-MAX5484.pdf
>>
>> Signed-off-by: Maury Anderson <maury.anderson@xxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Matthew Weber <matthew.weber@xxxxxxxxxxxxxxxxxxx>
>> ---
>>
>> This patch is a proposed implementation which compiles and
>> is based on the 5487 part.  The integration of the component
>> was cancelled but we wanted to still share this work hoping
>> that others might pick it up.
>>
>> ---
>>  .../bindings/iio/potentiometer/max5481.txt         |  23 +++
>>  drivers/iio/potentiometer/Kconfig                  |  12 ++
>>  drivers/iio/potentiometer/Makefile                 |   1 +
>>  drivers/iio/potentiometer/max5481.c                | 212 +++++++++++++++++++++
>>  4 files changed, 248 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/max5481.txt
>>  create mode 100644 drivers/iio/potentiometer/max5481.c
>>
>> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/max5481.txt b/Documentation/devicetree/bindings/iio/potentiometer/max5481.txt
>> new file mode 100644
>> index 0000000..3bc59b4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/potentiometer/max5481.txt
>> @@ -0,0 +1,23 @@
>> +* Maxim Linear-Taper Digital Potentiometer MAX5481-MAX5484
>> +
>> +The node for this driver must be a child node of a SPI controller, hence
>> +all mandatory properties described in
>> +
>> +        Documentation/devicetree/bindings/spi/spi-bus.txt
>> +
>> +must be specified.
>> +
>> +Required properties:
>> +     - compatible:   Must be one of the following, depending on the
>> +                     model:
>> +                     "maxim,max5481"
>> +                     "maxim,max5482"
>> +                     "maxim,max5483"
>> +                     "maxim,max5484"
>> +
>> +Example:
>> +max548x: max548x@0 {
>> +     compatible = "maxim,max5482";
>> +     spi-max-frequency = <7000000``>;
>
> there are two backticks ``?
>
>> +     reg = <0>; /* chip-select */
>> +};
>> diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
>> index 2e9da1c..872b25b 100644
>> --- a/drivers/iio/potentiometer/Kconfig
>> +++ b/drivers/iio/potentiometer/Kconfig
>> @@ -15,6 +15,18 @@ config DS1803
>>         To compile this driver as a module, choose M here: the
>>         module will be called ds1803.
>>
>> +config MAX5481
>> +        tristate "Maxim MAX5481-MAX5484 Digital Potentiometer driver"
>> +        depends on SPI
>> +        help
>> +          Say yes here to build support for the Maxim
>> +          MAX5481, MAX5482, MAX5483, MAX5484 digital potentiometer
>> +          chips.
>> +
>> +          To compile this driver as a module, choose M here: the
>> +          module will be called max5481.
>> +
>> +c
>
> there is an extra 'c'
>
>>  config MAX5487
>>          tristate "Maxim MAX5487/MAX5488/MAX5489 Digital Potentiometer driver"
>>          depends on SPI
>> diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
>> index 8adb58f..2260d40 100644
>> --- a/drivers/iio/potentiometer/Makefile
>> +++ b/drivers/iio/potentiometer/Makefile
>> @@ -4,6 +4,7 @@
>>
>>  # When adding new entries keep the list in alphabetical order
>>  obj-$(CONFIG_DS1803) += ds1803.o
>> +obj-$(CONFIG_MAX5481) += max5481.o
>>  obj-$(CONFIG_MAX5487) += max5487.o
>>  obj-$(CONFIG_MCP4131) += mcp4131.o
>>  obj-$(CONFIG_MCP4531) += mcp4531.o
>> diff --git a/drivers/iio/potentiometer/max5481.c b/drivers/iio/potentiometer/max5481.c
>> new file mode 100644
>> index 0000000..f3feefc
>> --- /dev/null
>> +++ b/drivers/iio/potentiometer/max5481.c
>> @@ -0,0 +1,212 @@
>> +/*
>> + * Maxim Integrated MAX5481-MAX5484 digital potentiometer driver
>> + * Copyright 2016 Rockwell Collins
>> + *
>> + * Datasheet:
>> + * http://datasheets.maximintegrated.com/en/ds/MAX5481-MAX5484.pdf
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the gnu general public license version 2 as
>> + * published by the free software foundation.
>> + *
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/of.h>
>> +#include <linux/acpi.h>
>> +
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/iio/iio.h>
>> +
>> +/* write wiper reg */
>> +#define MAX5481_WRITE_WIPER (0 << 4)
>> +/* copy wiper reg to NV reg */
>> +#define MAX5481_COPY_AB_TO_NV (2 << 4)
>> +/* copy NV reg to wiper reg */
>> +#define MAX5481_COPY_NV_TO_AB (3 << 4)
>> +
>> +#define MAX5481_MAX_POS    1023
>> +
>> +enum max5481_variant {
>> +     max5481,
>> +     max5482,
>> +     max5483,
>> +     max5484
>
> add , on the last line, chances are the list will be extended...
>
>> +};
>> +
>> +struct max5481_cfg {
>> +     int kohms;
>> +     int maxpos;
>> +};
>> +
>> +static const struct max5481_cfg max5481_cfg[] = {
>> +     [max5481] = { .kohms =  10, .maxpos = MAX5481_MAX_POS },
>> +     [max5482] = { .kohms =  50, .maxpos = MAX5481_MAX_POS },
>> +     [max5483] = { .kohms =  10, .maxpos = MAX5481_MAX_POS },
>> +     [max5484] = { .kohms =  50, .maxpos = MAX5481_MAX_POS },
>> +};
>
> what is the difference between parts?
>
>> +
>> +struct max5481_data {
>> +     struct spi_device *spi;
>> +     const struct max5481_cfg *cfg;
>> +};
>> +
>> +#define max5481_CHANNEL(ch) {                                        \
>
> MAX5481_CHANNEL
> is the ch parameter needed?
>
>> +     .type = IIO_RESISTANCE,                                 \
>> +     .indexed = 1,                                           \
>> +     .output = 1,                                            \
>> +     .channel = (ch),                                        \
>> +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),           \
>> +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),   \
>> +}
>> +
>> +static const struct iio_chan_spec max5481_channels[] = {
>> +     max5481_CHANNEL(0),
>> +};
>> +
>> +static int max5481_write_cmd(struct spi_device *spi, int cmd, int val)
>> +{
>> +     /* SPI Format from MAX5481-MAX5484 (19-3708; Rev 5; 4/10) pg 15 */
>> +     u8 msg[3] = {
>> +             (u8)cmd,
>
> cast (u8) needed?
>
>> +             (u8)((u16)val >> 2),
>> +             (u8)(val & 0x3) << 6
>> +     };
>> +
>> +     return spi_write(spi, msg, (cmd == MAX5481_WRITE_WIPER) ? sizeof(msg) : sizeof(u8));
>> +}
>> +
>> +static int max5481_read_raw(struct iio_dev *indio_dev,
>> +             struct iio_chan_spec const *chan,
>> +             int *val, int *val2, long mask)
>> +{
>> +     struct max5481_data *data = iio_priv(indio_dev);
>> +
>> +     if (mask != IIO_CHAN_INFO_SCALE)
>> +             return -EINVAL;
>> +
>> +     *val = 1000 * data->cfg->kohms;
>> +     *val2 = data->cfg->maxpos;
>> +
>> +     return IIO_VAL_FRACTIONAL;
>> +}
>> +
>> +static int max5481_write_raw(struct iio_dev *indio_dev,
>> +             struct iio_chan_spec const *chan,
>> +             int val, int val2, long mask)
>> +{
>> +     struct max5481_data *data = iio_priv(indio_dev);
>> +
>> +     if (mask != IIO_CHAN_INFO_RAW)
>> +             return -EINVAL;
>> +
>> +     if (val < 0 || val > data->cfg->maxpos)
>> +             return -EINVAL;
>> +
>> +     return max5481_write_cmd(data->spi, MAX5481_WRITE_WIPER, val);
>> +}
>> +
>> +static const struct iio_info max5481_info = {
>> +     .read_raw = max5481_read_raw,
>> +     .write_raw = max5481_write_raw,
>> +     .driver_module = THIS_MODULE,
>> +};
>> +
>> +static int max5481_probe(struct spi_device *spi)
>> +{
>> +     struct iio_dev *indio_dev;
>> +     struct max5481_data *data;
>> +     const struct spi_device_id *id = spi_get_device_id(spi);
>> +     int ret;
>> +
>> +     indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
>> +     if (!indio_dev)
>> +             return -ENOMEM;
>> +
>> +     dev_set_drvdata(&spi->dev, indio_dev);
>> +     data = iio_priv(indio_dev);
>> +
>> +     data->spi = spi;
>> +     data->cfg = &max5481_cfg[id->driver_data];
>> +
>> +     indio_dev->name = id->name;
>> +     indio_dev->dev.parent = &spi->dev;
>> +     indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> +     /* variant specific configuration */
>> +     indio_dev->info = &max5481_info;
>> +     indio_dev->channels = max5481_channels;
>> +     indio_dev->num_channels = ARRAY_SIZE(max5481_channels);
>> +
>> +     /* restore wiper from NV */
>> +     ret = max5481_write_cmd(data->spi, MAX5481_COPY_NV_TO_AB, 0);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return iio_device_register(indio_dev);
>> +}
>> +
>> +static int max5481_remove(struct spi_device *spi)
>> +{
>> +     struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
>> +     int ret;
>> +
>> +     /* save wiper reg to NV reg */
>> +     ret = max5481_write_cmd(spi, MAX5481_COPY_AB_TO_NV, 0);
>> +
>> +     iio_device_unregister(indio_dev);
>
> unregister first, then save
>
>> +
>> +     return ret;
>> +}
>> +
>> +static const struct spi_device_id max5481_id_table[] = {
>> +     { "MAX5481", max5481 },
>> +     { "MAX5482", max5482 },
>> +     { "MAX5483", max5483 },
>> +     { "MAX5484", max5484 },
>> +     { }
>> +};
>> +MODULE_DEVICE_TABLE(spi, max5481_id_table);
>> +
>> +#if defined(CONFIG_OF)
>> +static const struct of_device_id max5481_match[] = {
>> +     { .compatible = "maxim,max5481", .data = &max5481_cfg[max5481] },
>> +     { .compatible = "maxim,max5482", .data = &max5481_cfg[max5482] },
>> +     { .compatible = "maxim,max5483", .data = &max5481_cfg[max5483] },
>> +     { .compatible = "maxim,max5484", .data = &max5481_cfg[max5484] },
>> +     { }
>> +};
>> +MODULE_DEVICE_TABLE(of, max5481_match);
>> +#endif
>> +
>> +#if defined(CONFIG_ACPI)
>> +static const struct acpi_device_id max5481_acpi_match[] = {
>> +     { "MAX5481", max5481 },
>> +     { "MAX5482", max5482 },
>> +     { "MAX5483", max5483 },
>> +     { "MAX5484", max5484 },
>> +     { }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, max5481_acpi_match);
>> +#endif
>> +
>> +static struct spi_driver max5481_driver =
>> +{
>> +     .driver =
>> +     {
>> +             .name  = "max5481",
>> +             .owner = THIS_MODULE,
>> +             .of_match_table = of_match_ptr(max5481_match),
>> +             .acpi_match_table = ACPI_PTR(max5481_acpi_match),
>> +     },
>> +     .probe = max5481_probe,
>> +     .remove = max5481_remove,
>> +     .id_table = max5481_id_table,
>> +};
>> +
>> +module_spi_driver(max5481_driver);
>> +
>> +MODULE_AUTHOR("Maury Anderson <maury.anderson@xxxxxxxxxxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("max5481 SPI driver");
>> +MODULE_LICENSE("GPL v2");
>>
>
> --
>
> Peter Meerwald-Stadler
> +43-664-2444418 (mobile)



-- 
Matthew L Weber / Pr Software Engineer
Airborne Information Systems / Security Systems and Software / Secure Platforms
MS 131-100, C Ave NE, Cedar Rapids, IA, 52498, USA
www.rockwellcollins.com

Note: Any Export License Required Information and License Restricted
Third Party Intellectual Property (TPIP) content must be encrypted and
sent to matthew.weber@xxxxxxxxxxxxxxxxxxxxxxxx.
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux