Re: [PATCH v7 3/4] pdm: Imagination Technologies PDM DAC driver

[One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>]

On Wed, 7 Jan 2015 14:20:14 -0300
Ezequiel Garcia <ezequiel.garcia@imgtec.com> wrote:

> From: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
> 
> The Pistachio SOC from Imagination Technologies includes a Pulse Density
> Modulation DAC which produces a form of analogue output according to the
> relative density of output pulses to the intended analogue signal amplitude.
> Four PDM outputs are provided that can be used to control targets such as LCD
> backlight.


> +int img_pdm_channel_config(struct img_pdm_channel *chan, unsigned int val)
> +{
> +	struct img_pdm_device *pdm_dev;
> +
> +	if (!chan)
> +		return -EINVAL;

If this is a can't happen case then either let it crash or WARN_ON it so
that it doesn't get ignored and lead to nastier failures elsewhere

> +
> +static struct img_pdm_channel *img_pdm_channel_request(unsigned int pdm_id)
> +{
> +	unsigned int i;
> +	struct img_pdm_device *pdm_dev;
> +	struct img_pdm_channel *chan = NULL;
> +
> +	mutex_lock(&pdm_lock);
> +
> +	if (pdm_id < 0 || pdm_id >= IMG_NUM_PDM || !pdm_channels)
> +		return NULL;

You return with the mutex held in this case.



> +int img_pdm_channel_enable(struct img_pdm_channel *chan, bool state)
> +{
> +	struct img_pdm_device *pdm_dev;
> +
> +	if (!chan)
> +		return -EINVAL;

Same comment about hiding errors being bad

> +	pdm_dev = chan->pdm_dev;
> +
> +	if (!test_bit(PDM_CHANNEL_REQUESTED, &chan->flags)) {
> +		dev_err(&pdm_dev->pdev->dev, "channel not requested\n");
> +		return -EINVAL;
> +	}
> +
> +	if (state) {
> +		regmap_update_bits(pdm_dev->periph_regs,
> +				   PERIP_PWM_PDM_CONTROL,
> +				   PERIP_PWM_PDM_CONTROL_CH_MASK <<
> +				   PERIP_PWM_PDM_CONTROL_CH_SHIFT(chan->pdm_id),
> +				   1);
> +		set_bit(PDM_CHANNEL_ENABLED, &chan->flags);
> +	} else {
> +		regmap_write(pdm_dev->periph_regs,
> +			     PERIP_PDM0_VAL +
> +			     PERIP_PDM_CH_ADDR_SHIFT(chan->pdm_id), 0);
> +		regmap_update_bits(pdm_dev->periph_regs,
> +				   PERIP_PWM_PDM_CONTROL,
> +				   PERIP_PWM_PDM_CONTROL_CH_MASK <<
> +				   PERIP_PWM_PDM_CONTROL_CH_SHIFT(chan->pdm_id),
> +				   0);
> +		clear_bit(PDM_CHANNEL_ENABLED, &chan->flags);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(img_pdm_channel_enable);

You export these functions but they don't appear to do any internal
locking in them, so how is the caller expected to use them ?

If there is a reason no locking is needed in this case document why and
the assumptions.

> +
> +static ssize_t img_pdm_enable_read(struct kobject *kobj,
> +				   struct kobj_attribute *attr, char *buf)
> +{
> +	int ret;
> +	unsigned int ch_num;
> +	unsigned char kobj_name[2];
> +	struct platform_device *pdev;
> +	struct img_pdm_device *pdm_dev;
> +	struct img_pdm_channel *chan;
> +
> +	pdev = to_platform_device(kobj_to_dev(kobj->parent));
> +	pdm_dev = platform_get_drvdata(pdev);
> +	kobj_name[0] = *(kobj->name+3);
> +	kobj_name[1] = '\0';
> +
> +	ret = kstrtou32(kobj_name, 10, &ch_num);

This is C not java. 

	ch_num = kobj->name[3] - '0';

> +	if (ret) {
> +		dev_err(&pdev->dev, "could not parse channel number string\n");
> +		return ret;
> +	}
> +
> +	chan = &pdm_channels[ch_num];
> +	return sprintf(buf, "%d\n",
> +		       test_bit(PDM_CHANNEL_ENABLED, &chan->flags) ? 1 : 0);
> +}
> +
> +static ssize_t img_pdm_pulse_in_read(struct kobject *kobj,
> +				     struct kobj_attribute *attr, char *buf)
> +{
> +	int ret;
> +	unsigned int ch_num, val;
> +	unsigned char kobj_name[2];
> +	struct platform_device *pdev;
> +	struct img_pdm_device *pdm_dev;
> +	struct img_pdm_channel *chan;
> +
> +	pdev = to_platform_device(kobj_to_dev(kobj->parent));
> +	pdm_dev = platform_get_drvdata(pdev);
> +	kobj_name[0] = *(kobj->name+3);
> +	kobj_name[1] = '\0';
> +	ret = kstrtou32(kobj_name, 10, &ch_num);

Ditto (in fact why not just have a helper to range check and print the
error and share the string)

> +static ssize_t img_pdm_enable_write(struct kobject *kobj,
> +				    struct kobj_attribute *attr,
> +				    const char *buf, size_t size)
> +{
> +	int ret;
> +	unsigned int ch_num, enable;
> +	unsigned char kobj_name[2];
> +	struct platform_device *pdev;
> +	struct img_pdm_device *pdm_dev;
> +
> +	pdev = to_platform_device(kobj_to_dev(kobj->parent));
> +	pdm_dev = platform_get_drvdata(pdev);
> +
> +	kobj_name[0] = *(kobj->name+3);
> +	kobj_name[1] = '\0';
> +	ret = kstrtou32(kobj_name, 10, &ch_num);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not parse channel number string\n");
> +		return ret;
> +	}

And again

> +static ssize_t img_pdm_pulse_in_write(struct kobject *kobj,
> +				      struct kobj_attribute *attr,
> +				      const char *buf, size_t size)
> +{
> +	int ret;
> +	unsigned int pulse_in, ch_num;
> +	unsigned char kobj_name[2];
> +	struct platform_device *pdev;
> +	struct img_pdm_device *pdm_dev;
> +
> +	pdev = to_platform_device(kobj_to_dev(kobj->parent));
> +	pdm_dev = platform_get_drvdata(pdev);
> +
> +	kobj_name[0] = *(kobj->name+3);
> +	kobj_name[1] = '\0';
> +	ret = kstrtou32(kobj_name, 10, &ch_num);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not parse channel number string\n");
> +		return ret;
> +	}

and - you get the idea 8)