Date:	Tue, 23 Jun 2015 18:19:45 +0530
From:	Shobhit Kumar <kumar@...bhit.info>
To:	Varka Bhadram <varkabhadram@...il.com>
Cc:	Shobhit Kumar <shobhit.kumar@...el.com>,
	linux-pwm <linux-pwm@...r.kernel.org>,
	intel-gfx <intel-gfx@...ts.freedesktop.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	dri-devel <dri-devel@...ts.freedesktop.org>,
	linux-gpio <linux-gpio@...r.kernel.org>,
	Alexandre Courbot <gnurou@...il.com>,
	Paul Bolle <pebolle@...cali.nl>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Povilas Staniulis <wdmonster@...il.com>,
	Jani Nikula <jani.nikula@...el.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Paul Gortmaker <paul.gortmaker@...driver.com>,
	bloften80@...il.com, David Airlie <airlied@...ux.ie>,
	Chih-Wei Huang <cwhuang@...roid-x86.org>,
	Thierry Reding <thierry.reding@...il.com>,
	Daniel Vetter <daniel.vetter@...el.com>,
	Lee Jones <lee.jones@...aro.org>
Subject: Re: [Intel-gfx] [v2 5/7] pwm: crc: Add Crystalcove (CRC) PWM driver

On Mon, Jun 22, 2015 at 4:46 PM, Varka Bhadram <varkabhadram@...il.com> wrote:
> Hi Shobhit Kumar,
>
> On 06/22/2015 04:24 PM, Shobhit Kumar wrote:
>
>> The Crystalcove PMIC provides three PWM signals and this driver exports
>> one of them on the BYT platform which is used to control backlight for
>> DSI panel. This is platform device implementation of the drivers/mfd
>> cell device for CRC PMIC.
>>
>> v2: Use the existing config callback with duty_ns and period_ns(Thierry)
>>
>> v3: Correct the subject line (Lee jones)
>>
>> v4: Address comment by Thierry & Paul
>>      - Commit message update and fixes for few syntax errors
>>      - Add PWM_CRC in Kconfig and Makefile sorted alphabetically
>>      - Use the PWM_BASE_CLK as 6000000 for better code readability
>>      - Remove the redundant rule of three while calculating pwm level
>>      - Use the platform_device in pwm_chip
>>      - Use builin_platform_driver
>>
>> CC: Samuel Ortiz <sameo@...ux.intel.com>
>> Cc: Linus Walleij <linus.walleij@...aro.org>
>> Cc: Alexandre Courbot <gnurou@...il.com>
>> Cc: Thierry Reding <thierry.reding@...il.com>
>> Cc: Paul Bolle <pebolle@...cali.nl>
>> Cc: Paul Gortmaker <paul.gortmaker@...driver.com>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@...el.com>
>
>
> (...)
>
>> +
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/intel_soc_pmic.h>
>> +#include <linux/pwm.h>
>> +
>> +#define PWM0_CLK_DIV           0x4B
>> +#define  PWM_OUTPUT_ENABLE     (1 << 7)
>
>
> Can't be BIT() macro ?
>

Can be done.

>> +#define  PWM_DIV_CLK_0         0x00 /* DIVIDECLK = BASECLK */
>> +#define  PWM_DIV_CLK_100       0x63 /* DIVIDECLK = BASECLK/100 */
>> +#define  PWM_DIV_CLK_128       0x7F /* DIVIDECLK = BASECLK/128 */
>> +
>> +#define PWM0_DUTY_CYCLE                0x4E
>> +#define BACKLIGHT_EN           0x51
>
>
> (...)
>
>
>> +static int crystalcove_pwm_probe(struct platform_device *pdev)
>> +{
>> +       struct crystalcove_pwm *pwm;
>> +       int retval;
>> +       struct device *dev = pdev->dev.parent;
>> +       struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
>> +
>> +       pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
>> +       if (!pwm)
>> +               return -ENOMEM;
>> +
>> +       pwm->chip.dev = &pdev->dev;
>> +       pwm->chip.ops = &crc_pwm_ops;
>> +       pwm->chip.base = -1;
>> +       pwm->chip.npwm = 1;
>> +
>> +       /* get the PMIC regmap */
>> +       pwm->regmap = pmic->regmap;
>> +
>> +       retval = pwmchip_add(&pwm->chip);
>> +       if (retval < 0)
>> +               return retval;
>> +
>> +       platform_set_drvdata(pdev, pwm);
>> +
>
>
> If you can change this oder we can simply do something like this:
>
>         platform_set_drvdata(pdev, pwm);
>
>         return pwmchip_add(&pwm->chip);
>

Okay. seems better.

>> +       return 0;
>> +}
>> +
>> +static int crystalcove_pwm_remove(struct platform_device *pdev)
>> +{
>> +       struct crystalcove_pwm *pwm = platform_get_drvdata(pdev);
>> +       int retval;
>> +
>> +       retval = pwmchip_remove(&pwm->chip);
>> +       if (retval < 0)
>> +               return retval;
>> +
>> +       dev_dbg(&pdev->dev, "crc-pwm driver removed\n");
>
>
> This debug message may not be required  :-)
>
> you can directly do:
>
>         return pwmchip_remove(&pwm->chip);

Yeah, will update.

Regards
Shobhit

>
>> +
>> +       return 0;
>> +}
>> +
>> +static struct platform_driver crystalcove_pwm_driver = {
>> +       .probe = crystalcove_pwm_probe,
>> +       .remove = crystalcove_pwm_remove,
>> +       .driver = {
>> +               .name = "crystal_cove_pwm",
>> +       },
>> +};
>> +
>> +builtin_platform_driver(crystalcove_pwm_driver);
>
>
>
> --
> Best regards,
> Varka Bhadram.
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@...ts.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists