On 14/01/15 11:30, Opensource [Adam Thomson] wrote:
> On January 10, 2015 22:19, Jonathan Cameron wrote:
>
>> On 07/01/15 16:03, Opensource [Adam Thomson] wrote:
>>> On January 4, 2015 17:22, Jonathan Cameron wrote:
>>>
>>>> On 22/12/14 16:51, Adam Thomson wrote:
>>>>> This patch adds support for DA9150 Charger & Fuel-Gauge IC GPADC.
>>>>>
>>>>> Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@xxxxxxxxxxx>
>>>> One last query from me.
>>>>
>>>> Using the extended channel names in IIO is only really appropriate when
>>>> they don't correspond to simple pins on the side of the chip. For those
>>>> just drop the extname bit. Some of the channels you have here, definitely
>>>> need them though.
>>>>
>>>> Drop those first 4 or convince me otherwise and add
>>>> Acked-by: Jonathan Cameron <jic23@xxxxxxxxxx>
>>>
>>> Have added responses below. If the comments are accepted then I'll respin
>>> and add you're 'Acked-by'. Is that ok?
>>>
>> Not accepted as yet :)
>
> Yeah, that's why I added the 'if'. :)
>
>>>>> +#define DA9150_GPADC_CHANNEL_PROCESSED(_id, _hw_id, _type,
>> _ext_name)
>>>> \
>>>>> + DA9150_GPADC_CHANNEL(_id, _hw_id, _type, \
>>>>> + BIT(IIO_CHAN_INFO_PROCESSED), _ext_name)
>>>>> +
>>>>> +/* Supported channels */
>>>>> +static const struct iio_chan_spec da9150_gpadc_channels[] = {
>>>>> + DA9150_GPADC_CHANNEL_PROCESSED(GPIOA, GPIOA_6V, IIO_VOLTAGE,
>>>> "GPIOA"),
>>>>> + DA9150_GPADC_CHANNEL_PROCESSED(GPIOB, GPIOB_6V, IIO_VOLTAGE,
>>>> "GPIOB"),
>>>>> + DA9150_GPADC_CHANNEL_PROCESSED(GPIOC, GPIOC_6V, IIO_VOLTAGE,
>>>> "GPIOC"),
>>>>> + DA9150_GPADC_CHANNEL_PROCESSED(GPIOD, GPIOD_6V, IIO_VOLTAGE,
>>>> "GPIOD"),
>>>> I'm not sure some of these really deserve extended names. Those are usually
>>>> reserved for naming strange internal adc channels etc. These first 4 are
>>>> presumably just for input pins? Those should just be channels 0..3
>>>> On another note, unless you want really weird sysfs attribute names, the
>>>> extended names want to be lowercase.
>>>>
>>>
>>> I'd prefer to keep the names because the input pins are muxed with GPIOs of the
>>> chip, so thought it sensible to show that this is the case. Am happy to change
>>> to lower-case to follow convention.
>> Hmm. It's a bit of an oddity as the point of the naming
>> is about the uses, not which pins they are on. If we exposed the
>> 'datasheet_name' parameter directly rather than just using it internally
>> I'd suggest relying on that - but clearly you want it to be apparent
>> in the interface. Whether that is useful is the question I'd raise
>> here (and is the reason datasheet_name is not exposed.
>>
>> The obvious question is does userspace care? Answer is probably not.
>>
>> It cares what is being measured but this is about what pins it is
>> on and doesn't provide any information on what is connected to them.
>>
>
> Surely it helps when using sysfs to access whatever is connected to one of
> those pins if we label the pin with something meaningful? If say you have a
> device connected to GPIC of the charger IC, it's easier to work out which ADC
> channel you need to access through sysfs if the naming is as I have now, rather
> than some arbitrary number which doesn't necessarily tally to the channel in the
> datasheet. You'd then need to look at the code and work out which channel number
> GPIOC actually was. Or am I just missing something here? :)
Not really for the vast majority of users. They tend not to have a detailed
board layout in front of them. It's more interesting if you know 'what' they
are measuring (hence we do use these names when that is true - such as
internal voltage measurements).
The numbers almost never tally with the datasheet, but then datasheet numbering
has a habit of being inconsistent as well!
>
>>
>>>
>>>>> + DA9150_GPADC_CHANNEL_PROCESSED(IBUS, IBUS_SENSE, IIO_CURRENT,
>>>> "IBUS"),
>>>>> + DA9150_GPADC_CHANNEL_PROCESSED(VBUS, VBUS_DIV_, IIO_VOLTAGE,
>>>> "VBUS"),
>>>>> + DA9150_GPADC_CHANNEL_RAW(ID, ID, IIO_VOLTAGE, "ID"),
>>>> You hae an identifier voltage? That's certainly unusual but if so - fair enough
>>>> and it defintely needs the extname!
>>>
>>> Thanks for pointing that out. Having checked again, this is not needed and can
>>> be dispensed with.
>>>
>>>>> + DA9150_GPADC_CHANNEL_PROCESSED(VSYS, VSYS, IIO_VOLTAGE, "VSYS"),
>>>>> + DA9150_GPADC_CHANNEL_SCALED(VBAT, VBAT, IIO_VOLTAGE, "VBAT"),
>>>>> + DA9150_GPADC_CHANNEL_RAW(TBAT, TBAT, IIO_VOLTAGE, "TBAT"),
>>>>> + DA9150_GPADC_CHANNEL_SCALED(TJUNC_CORE, TJUNC_CORE,
>>>> IIO_TEMP,
>>>>> + "TJUNC_CORE"),
>>>> tjunc_core is a good use of extname ;)
>>>>> + DA9150_GPADC_CHANNEL_SCALED(TJUNC_OVP, TJUNC_OVP, IIO_TEMP,
>>>>> + "TJUNC_OVP"),
>>>>> +};
>>>>> +
>>>>> +/* Default maps used by da9150-charger */
>>>>> +static struct iio_map da9150_gpadc_default_maps[] = {
>>>>> + {
>>>>> + .consumer_dev_name = "da9150-charger",
>>>>> + .consumer_channel = "CHAN_IBUS",
>>>>> + .adc_channel_label = "IBUS",
>>>>> + },
>>>>> + {
>>>>> + .consumer_dev_name = "da9150-charger",
>>>>> + .consumer_channel = "CHAN_VBUS",
>>>>> + .adc_channel_label = "VBUS",
>>>>> + },
>>>>> + {
>>>>> + .consumer_dev_name = "da9150-charger",
>>>>> + .consumer_channel = "CHAN_TJUNC",
>>>>> + .adc_channel_label = "TJUNC_CORE",
>>>>> + },
>>>>> + {
>>>>> + .consumer_dev_name = "da9150-charger",
>>>>> + .consumer_channel = "CHAN_VBAT",
>>>>> + .adc_channel_label = "VBAT",
>>>>> + },
>>>>> + {},
>>>>> +};
>>>>> +
>>>>> +static int da9150_gpadc_probe(struct platform_device *pdev)
>>>>> +{
>>>>> + struct device *dev = &pdev->dev;
>>>>> + struct da9150 *da9150 = dev_get_drvdata(dev->parent);
>>>>> + struct da9150_gpadc *gpadc;
>>>>> + struct iio_dev *indio_dev;
>>>>> + int irq, ret;
>>>>> +
>>>>> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*gpadc));
>>>>> + if (!indio_dev) {
>>>>> + dev_err(&pdev->dev, "Failed to allocate IIO device\n");
>>>>> + return -ENOMEM;
>>>>> + }
>>>>> + gpadc = iio_priv(indio_dev);
>>>>> +
>>>>> + platform_set_drvdata(pdev, indio_dev);
>>>>> + gpadc->da9150 = da9150;
>>>>> + gpadc->dev = dev;
>>>>> + mutex_init(&gpadc->lock);
>>>>> + init_completion(&gpadc->complete);
>>>>> +
>>>>> + irq = platform_get_irq_byname(pdev, "GPADC");
>>>>> + if (irq < 0) {
>>>>> + dev_err(dev, "Failed to get IRQ: %d\n", irq);
>>>>> + return irq;
>>>>> + }
>>>>> +
>>>>> + ret = devm_request_threaded_irq(dev, irq, NULL, da9150_gpadc_irq,
>>>>> + IRQF_ONESHOT, "GPADC", gpadc);
>>>>> + if (ret) {
>>>>> + dev_err(dev, "Failed to request IRQ %d: %d\n", irq, ret);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + ret = iio_map_array_register(indio_dev, da9150_gpadc_default_maps);
>>>>> + if (ret) {
>>>>> + dev_err(dev, "Failed to register IIO maps: %d\n", ret);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + indio_dev->name = dev_name(dev);
>>>>> + indio_dev->dev.parent = dev;
>>>>> + indio_dev->dev.of_node = pdev->dev.of_node;
>>>>> + indio_dev->info = &da9150_gpadc_info;
>>>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>>>> + indio_dev->channels = da9150_gpadc_channels;
>>>>> + indio_dev->num_channels = ARRAY_SIZE(da9150_gpadc_channels);
>>>>> +
>>>>> + ret = iio_device_register(indio_dev);
>>>>> + if (ret) {
>>>>> + dev_err(dev, "Failed to register IIO device: %d\n", ret);
>>>>> + goto iio_map_unreg;
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +
>>>>> +iio_map_unreg:
>>>>> + iio_map_array_unregister(indio_dev);
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static int da9150_gpadc_remove(struct platform_device *pdev)
>>>>> +{
>>>>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>>>> +
>>>>> + iio_device_unregister(indio_dev);
>>>>> + iio_map_array_unregister(indio_dev);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static struct platform_driver da9150_gpadc_driver = {
>>>>> + .driver = {
>>>>> + .name = "da9150-gpadc",
>>>>> + },
>>>>> + .probe = da9150_gpadc_probe,
>>>>> + .remove = da9150_gpadc_remove,
>>>>> +};
>>>>> +
>>>>> +module_platform_driver(da9150_gpadc_driver);
>>>>> +
>>>>> +MODULE_DESCRIPTION("GPADC Driver for DA9150");
>>>>> +MODULE_AUTHOR("Adam Thomson
>>>> <Adam.Thomson.Opensource@xxxxxxxxxxx>");
>>>>> +MODULE_LICENSE("GPL");
>>>>> --
>>>>> 1.9.3
>>>>>
--
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
