From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <5475085E.8090806@gmx.de> Date: Tue, 25 Nov 2014 23:53:18 +0100 From: Hartmut Knaack MIME-Version: 1.0 To: Ezequiel Garcia , Andrew Bresticker , James Hartley , Lars-Peter Clausen , Jonathan Cameron CC: linux-iio@vger.kernel.org, devicetree@vger.kernel.org, robh+dt@kernel.org, Pawel.Moll@arm.com, Mark.Rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, Naidu.Tellapati@imgtec.com, Phani Movva Subject: Re: [PATCH v3 1/3] iio: adc: Cosmic Circuits 10001 ADC driver References: <1415888044-16635-1-git-send-email-ezequiel.garcia@imgtec.com> <1415888044-16635-2-git-send-email-ezequiel.garcia@imgtec.com> <546FE3A7.6040308@gmx.de> <5474C062.2020604@imgtec.com> <5474F7A5.6030008@gmx.de> <5474FCC8.8080906@imgtec.com> In-Reply-To: <5474FCC8.8080906@imgtec.com> Content-Type: text/plain; charset=ISO-8859-15 List-ID: Ezequiel Garcia schrieb am 25.11.2014 23:03: > > > On 11/25/2014 06:41 PM, Hartmut Knaack wrote: >>> >>> Unless I'm missing something, that's exactly what XOR does. >>> >>> Example: >>> >>> reserved_channels = 0x2; >>> channel_map = 0x7 ^ reserved_channels -> 0x5 >>> >> You miss to check ret for a valid range, which you don't need to do when masking out channels. >> Example: >> reserved_channels = 0x8; >> channel_map = 0x7 ^ reserved_channels -> 0xf >> And this is actually happening in the current revision (with the wrong channel_map assignment), that a reserved channel in DT would become usable. > > Right, definitely a check is needed. No, that's not needed when using AND, as it can not add bits in the channel map. The following will do the job: adc_dev->channel_map &= ~ret; > >>>>> + >>>>> + adc_dev->reg = devm_regulator_get(&pdev->dev, "vref"); >>>>> + if (IS_ERR_OR_NULL(adc_dev->reg)) >>>>> + return -EINVAL; >>>> if (IS_ERR(adc_dev->reg)) >>>> return PTR_ERR(adc_dev->reg); >>> >>> Are you sure? What if devm_regulator_get() returns NULL? >>> >> I had a look at it, but couldn't figure out a condition in which it would return NULL. But I'd be happy to be informed of anything different. > > If CONFIG_REGULATOR=n you get NULL there (although I added a select REGULATOR > on the v4 I just posted). > Indeed, thanks. Now I'm thinking if it would make sense to handle this special case in a special way (as it would be caused by an invalid kernel config). Well, I guess, I leave it up to you. >>>>> + >>>>> + ret = regulator_enable(adc_dev->reg); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + indio_dev->dev.parent = &pdev->dev; >>>>> + indio_dev->name = dev_name(&pdev->dev); >>>>> + indio_dev->info = &cc10001_adc_info; >>>>> + indio_dev->modes = INDIO_DIRECT_MODE; >>>>> + >>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>> + adc_dev->reg_base = devm_ioremap_resource(&pdev->dev, res); >>>>> + if (IS_ERR(adc_dev->reg_base)) >>>> Need to put error code into ret. >>> >>> Right. >>> >>>>> + goto err_disable_reg; >>>>> + >>>>> + adc_dev->adc_clk = devm_clk_get(&pdev->dev, "adc"); >>>>> + if (IS_ERR(adc_dev->adc_clk)) { >>>>> + dev_err(&pdev->dev, "Failed to get the clock\n"); >>>> Need to put error code into ret. >>>>> + goto err_disable_reg; >>>>> + } >>>>> + >>>>> + ret = clk_prepare_enable(adc_dev->adc_clk); >>>>> + if (ret) { >>>>> + dev_err(&pdev->dev, "Failed to enable the clock\n"); >>>>> + goto err_disable_reg; >>>>> + } >>>>> + >>>>> + adc_clk_rate = clk_get_rate(adc_dev->adc_clk); >>>>> + if (!adc_clk_rate) { >>>>> + ret = -EINVAL; >>>>> + dev_err(&pdev->dev, "null clock rate!\n"); >>>> Start message with upper case? >>> >>> Actually, I'd rather make the others start with lower case. >> Fair enough, unless they are full sentences like here. My english grammar tells me they should start with capital letters. > > Yeah, but it's not the start of the message, as it's prefixed > with the device stuff. > >>> >>>>> + goto err_disable_clk; >>>>> + } >>>>> + >>>>> + adc_dev->eoc_delay_ns = NSEC_PER_SEC / adc_clk_rate; >>>>> + adc_dev->start_delay_ns = adc_dev->eoc_delay_ns * CC10001_WAIT_CYCLES; >>>>> + >>>>> + /* Setup the ADC channels available on the device */ >>>>> + ret = cc10001_adc_channel_init(indio_dev); >>>>> + if (ret < 0) { >>>>> + dev_err(&pdev->dev, "Could not initialize the channels.\n"); >>>>> + goto err_disable_clk; >>>>> + } >>>>> + >>>>> + mutex_init(&adc_dev->lock); >>>>> + >>>>> + ret = iio_triggered_buffer_setup(indio_dev, NULL, >>>>> + &cc10001_adc_trigger_h, NULL); >>>>> + if (ret < 0) >>>>> + goto err_disable_clk; >>>>> + >>>>> + ret = iio_device_register(indio_dev); >>>>> + if (ret < 0) >>>>> + goto err_cleanup_buffer; >>>>> + >>>>> + platform_set_drvdata(pdev, indio_dev); >>>> Move this above iio_device_register. >>> >>> What for? >> To make iio_device_register the last operation of the probe. > > I really don't think it matters, since platform_get_drvdata > is only called in the remove. > > I'll move it if you think it's worth it, though. > > Thanks for the review! > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hartmut Knaack Subject: Re: [PATCH v3 1/3] iio: adc: Cosmic Circuits 10001 ADC driver Date: Tue, 25 Nov 2014 23:53:18 +0100 Message-ID: <5475085E.8090806@gmx.de> References: <1415888044-16635-1-git-send-email-ezequiel.garcia@imgtec.com> <1415888044-16635-2-git-send-email-ezequiel.garcia@imgtec.com> <546FE3A7.6040308@gmx.de> <5474C062.2020604@imgtec.com> <5474F7A5.6030008@gmx.de> <5474FCC8.8080906@imgtec.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7BIT Return-path: In-Reply-To: <5474FCC8.8080906-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> Sender: linux-iio-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Ezequiel Garcia , Andrew Bresticker , James Hartley , Lars-Peter Clausen , Jonathan Cameron Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, Pawel.Moll-5wv7dgnIgG8@public.gmane.org, Mark.Rutland-5wv7dgnIgG8@public.gmane.org, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org, galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org, Phani Movva List-Id: devicetree@vger.kernel.org Ezequiel Garcia schrieb am 25.11.2014 23:03: > > > On 11/25/2014 06:41 PM, Hartmut Knaack wrote: >>> >>> Unless I'm missing something, that's exactly what XOR does. >>> >>> Example: >>> >>> reserved_channels = 0x2; >>> channel_map = 0x7 ^ reserved_channels -> 0x5 >>> >> You miss to check ret for a valid range, which you don't need to do when masking out channels. >> Example: >> reserved_channels = 0x8; >> channel_map = 0x7 ^ reserved_channels -> 0xf >> And this is actually happening in the current revision (with the wrong channel_map assignment), that a reserved channel in DT would become usable. > > Right, definitely a check is needed. No, that's not needed when using AND, as it can not add bits in the channel map. The following will do the job: adc_dev->channel_map &= ~ret; > >>>>> + >>>>> + adc_dev->reg = devm_regulator_get(&pdev->dev, "vref"); >>>>> + if (IS_ERR_OR_NULL(adc_dev->reg)) >>>>> + return -EINVAL; >>>> if (IS_ERR(adc_dev->reg)) >>>> return PTR_ERR(adc_dev->reg); >>> >>> Are you sure? What if devm_regulator_get() returns NULL? >>> >> I had a look at it, but couldn't figure out a condition in which it would return NULL. But I'd be happy to be informed of anything different. > > If CONFIG_REGULATOR=n you get NULL there (although I added a select REGULATOR > on the v4 I just posted). > Indeed, thanks. Now I'm thinking if it would make sense to handle this special case in a special way (as it would be caused by an invalid kernel config). Well, I guess, I leave it up to you. >>>>> + >>>>> + ret = regulator_enable(adc_dev->reg); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + indio_dev->dev.parent = &pdev->dev; >>>>> + indio_dev->name = dev_name(&pdev->dev); >>>>> + indio_dev->info = &cc10001_adc_info; >>>>> + indio_dev->modes = INDIO_DIRECT_MODE; >>>>> + >>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>> + adc_dev->reg_base = devm_ioremap_resource(&pdev->dev, res); >>>>> + if (IS_ERR(adc_dev->reg_base)) >>>> Need to put error code into ret. >>> >>> Right. >>> >>>>> + goto err_disable_reg; >>>>> + >>>>> + adc_dev->adc_clk = devm_clk_get(&pdev->dev, "adc"); >>>>> + if (IS_ERR(adc_dev->adc_clk)) { >>>>> + dev_err(&pdev->dev, "Failed to get the clock\n"); >>>> Need to put error code into ret. >>>>> + goto err_disable_reg; >>>>> + } >>>>> + >>>>> + ret = clk_prepare_enable(adc_dev->adc_clk); >>>>> + if (ret) { >>>>> + dev_err(&pdev->dev, "Failed to enable the clock\n"); >>>>> + goto err_disable_reg; >>>>> + } >>>>> + >>>>> + adc_clk_rate = clk_get_rate(adc_dev->adc_clk); >>>>> + if (!adc_clk_rate) { >>>>> + ret = -EINVAL; >>>>> + dev_err(&pdev->dev, "null clock rate!\n"); >>>> Start message with upper case? >>> >>> Actually, I'd rather make the others start with lower case. >> Fair enough, unless they are full sentences like here. My english grammar tells me they should start with capital letters. > > Yeah, but it's not the start of the message, as it's prefixed > with the device stuff. > >>> >>>>> + goto err_disable_clk; >>>>> + } >>>>> + >>>>> + adc_dev->eoc_delay_ns = NSEC_PER_SEC / adc_clk_rate; >>>>> + adc_dev->start_delay_ns = adc_dev->eoc_delay_ns * CC10001_WAIT_CYCLES; >>>>> + >>>>> + /* Setup the ADC channels available on the device */ >>>>> + ret = cc10001_adc_channel_init(indio_dev); >>>>> + if (ret < 0) { >>>>> + dev_err(&pdev->dev, "Could not initialize the channels.\n"); >>>>> + goto err_disable_clk; >>>>> + } >>>>> + >>>>> + mutex_init(&adc_dev->lock); >>>>> + >>>>> + ret = iio_triggered_buffer_setup(indio_dev, NULL, >>>>> + &cc10001_adc_trigger_h, NULL); >>>>> + if (ret < 0) >>>>> + goto err_disable_clk; >>>>> + >>>>> + ret = iio_device_register(indio_dev); >>>>> + if (ret < 0) >>>>> + goto err_cleanup_buffer; >>>>> + >>>>> + platform_set_drvdata(pdev, indio_dev); >>>> Move this above iio_device_register. >>> >>> What for? >> To make iio_device_register the last operation of the probe. > > I really don't think it matters, since platform_get_drvdata > is only called in the remove. > > I'll move it if you think it's worth it, though. > > Thanks for the review! >