On Wed, 7 Jan 2015 14:20:14 -0300
Ezequiel Garcia <ezequiel.gar...@imgtec.com> wrote:
> From: Naidu Tellapati <naidu.tellap...@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)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
