On 28/01/17 15:12, Quentin Schulz wrote:
> Hi Jonathan
>
> On 28/01/2017 15:49, Jonathan Cameron wrote:
>> On 27/01/17 08:54, Quentin Schulz wrote:
>>> The X-Powers AXP20X and AXP22X PMICs have multiple ADCs. They expose the
>>> battery voltage, battery charge and discharge currents, AC-in and VBUS
>>> voltages and currents, 2 GPIOs muxable in ADC mode and PMIC temperature.
>>>
>>> This adds support for most of AXP20X and AXP22X ADCs.
>>>
>>> Signed-off-by: Quentin Schulz <quentin.schulz@xxxxxxxxxxxxxxxxxx>
>> Pretty good, but not everything seems to be cleaned up on error paths
>> in probe.
>>
>> A few other suggestions / questions inline.
>>
>> Jonathan
>>> ---
> [...]
>>> +static int axp20x_adc_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan, int *val)
>>> +{
>>> + struct axp20x_adc_iio *info = iio_priv(indio_dev);
>>> + int size = 12;
>>> +
>>> + switch (chan->type) {
>>> + case IIO_CURRENT:
>>> + /*
>>> + * Unlike the Chinese datasheets tell, the charging current is
>>> + * stored on 12 bits, not 13 bits.
>>> + */
>>> + if (chan->channel == AXP20X_BATT_DISCHRG_I)
>>> + size = 13;
>> Given I don't think you can get here without it being current, voltage or temp;
>> couldn't this be done more cleanly with
>> if ((chan->type == IIO_CURRENT) && (chan->channel == AXP20X_BAT_DISCHRG_I))
>> size = 13;
>>
>> and have the rest in the normal code flow?
>>
>
> Indeed.
>
>>> + case IIO_VOLTAGE:
>>> + case IIO_TEMP:
>>> + *val = axp20x_read_variable_width(info->regmap, chan->address,
>>> + size);
>>> + if (*val < 0)
>>> + return *val;
>>> +
>>> + return IIO_VAL_INT;
>>> +
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +}
> [...]
>>> +static int axp22x_adc_scale(struct iio_chan_spec const *chan, int *val,
>>> + int *val2)
>>> +{
>>> + switch (chan->type) {
>>> + case IIO_VOLTAGE:
>>> + if (chan->channel != AXP22X_BATT_V)
>>> + return -EINVAL;
>>> +
>>> + *val = 1;
>>> + *val2 = 100000;
>>> + return IIO_VAL_INT_PLUS_MICRO;
>> A fixed scale of 1.1x? (just checking)
>
> Yes, there is only one voltage exposed for AXP22X PMICs: the battery
> voltage which has a scale of 1.1 (for mV).
Fair enough - just seemed an oddly specific number!
>
> [...]
>>> +static int axp20x_probe(struct platform_device *pdev)
>>> +{
>>> + struct axp20x_adc_iio *info;
>>> + struct iio_dev *indio_dev;
>>> + struct axp20x_dev *axp20x_dev;
>>> + int ret;
>>> +
>>> + axp20x_dev = dev_get_drvdata(pdev->dev.parent);
>>> +
>>> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
>>> + if (!indio_dev)
>>> + return -ENOMEM;
>>> +
>>> + info = iio_priv(indio_dev);
>>> + platform_set_drvdata(pdev, indio_dev);
>>> +
>>> + info->regmap = axp20x_dev->regmap;
>>> + indio_dev->name = dev_name(&pdev->dev);
>> Not sure on this name - what does end up as? Expected to be
>> a description of the part so in this case something like axp209-adc.
>> I've been lax at picking up on this in the past and it's led to some
>> crazy naming that is no use at all to userspace. Basically this
>> name just provides a convenient user readable name for userspace apps to
>> use.
>>
>
> ACK. Should we have a different name for AXP20X and AXP22X PMICs?
ideally yes.
>
>>> + indio_dev->dev.parent = &pdev->dev;
>>> + indio_dev->dev.of_node = pdev->dev.of_node;
>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>> +
>>> + info->data = (struct axp_data *)of_device_get_match_data(&pdev->dev);
>>> +
>>> + indio_dev->info = info->data->iio_info;
>>> + indio_dev->num_channels = info->data->num_channels;
>>> + indio_dev->channels = info->data->channels;
>>> +
>>> + /* Enable the ADCs on IP */
>>> + regmap_write(info->regmap, AXP20X_ADC_EN1, info->data->adc_en1_mask);
>>> +
>>> + if (info->data->adc_en2)
>>> + /* Enable GPIO0/1 and internal temperature ADCs */
>>> + regmap_update_bits(info->regmap, AXP20X_ADC_EN2,
>>> + AXP20X_ADC_EN2_MASK, AXP20X_ADC_EN2_MASK);
>>> +
>>> + /* Configure ADCs rate */
>>> + regmap_update_bits(info->regmap, AXP20X_ADC_RATE, AXP20X_ADC_RATE_MASK,
>>> + info->data->adc_rate(100));
>>> +
>>> + ret = iio_map_array_register(indio_dev, info->data->maps);
>>> + if (ret < 0) {
>>> + dev_err(&pdev->dev, "failed to register IIO maps: %d\n", ret);
>> This should be disabling channels.
>
> You mean disabling ADCs as it is done in the remove? If so, indeed.
yes.
>
>>> + return ret;
>>> + }
>>> +
>>> + ret = iio_device_register(indio_dev);
>>> + if (ret < 0) {
>>> + dev_err(&pdev->dev, "could not register the device\n");
>>> + regmap_write(info->regmap, AXP20X_ADC_EN1, 0);
>>> +
>>> + if (info->data->adc_en2)
>>> + regmap_write(info->regmap, AXP20X_ADC_EN2, 0);
>> I'd expect to see a complete unwind of what has been done earlier in probe
>> including iio_map_array_unregister.
>>
>> The traditional goto error* approach is probably worth having here to
>> make sure the unwind makes sense.
>
> Indeed.
>
>>> +
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int axp20x_remove(struct platform_device *pdev)
>>> +{
>>> + struct axp20x_adc_iio *info;
>>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>> +
>>> + info = iio_priv(indio_dev);
>>> +
>>> + iio_device_unregister(indio_dev);
>> iio_map_array_unregister?
>
> Yes. I see iio_device_unregister already disable all buffers, why not
> unregistering the map array as well in this function?
Mostly about symmetry - it's easier to be sure things are right
if there are explicit calls to set stuff up and then to unwind it.
Jonathan
>
> [...]
> Thanks,
> Quentin
>
--
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
