From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753581Ab3B1Lwf (ORCPT ); Thu, 28 Feb 2013 06:52:35 -0500 Received: from slimlogic.co.uk ([89.16.172.20]:47576 "EHLO slimlogic.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753308Ab3B1Lwd (ORCPT ); Thu, 28 Feb 2013 06:52:33 -0500 Message-ID: <512F44FD.2040104@slimlogic.co.uk> Date: Thu, 28 Feb 2013 11:52:29 +0000 From: Ian Lartey User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130106 Thunderbird/17.0.2 MIME-Version: 1.0 To: Laxman Dewangan CC: "linux-kernel@vger.kernel.org" , "linux-leds@vger.kernel.org" , "devicetree-discuss@lists.ozlabs.org" , "j-keerthy@ti.com" , "gg@slimlogic.co.uk" , "cooloney@gmail.com" , "rpurdie@rpsys.net" , "grant.likely@secretlab.ca" , "rob.herring@calxeda.com" , "sameo@linux.intel.com" , "broonie@opensource.wolfsonmicro.com" Subject: Re: [PATCH 1/2] leds: Add support for Palmas LEDs References: <1361970800-31460-1-git-send-email-ian@slimlogic.co.uk> <512EEECF.5090409@nvidia.com> In-Reply-To: <512EEECF.5090409@nvidia.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 28/02/13 05:44, Laxman Dewangan wrote: > On Wednesday 27 February 2013 06:43 PM, Ian Lartey wrote: >> The Palmas familly of chips has LED support. This is not always muxed >> to output pins so depending on the setting of the mux this driver >> will create the appropriate LED class devices. >> >> Signed-off-by: Graeme Gregory >> Signed-off-by: Ian Lartey >> --- > > Patch 1 and 2 can be merged together as this is makefile, Kconfig and > driver addition. This could cause a merge conflict for anyone adding files/drivers in drivers/leds so it's safer to separate the Kconfig and Makefile from the actual drver source code so that can go in conflict free. > > >> + >> +static int palmas_led_read(struct palmas_leds_data *leds, unsigned >> int reg, >> + unsigned int *dest) >> +{ >> + unsigned int addr; >> + >> + addr = PALMAS_BASE_TO_REG(PALMAS_LED_BASE, reg); > This is not require. Infact doing this will not work. Yes, of course, thanks for that. > > palmas_read already take care of proper offset. > unsigned int addr = PALMAS_BASE_TO_REG(base, reg); > int slave_id = PALMAS_BASE_TO_SLAVE(base); > > return regmap_read(palmas->regmap[slave_id], addr, val); > > > same for other places also. ditto. > > >> >> +static void palmas_leds_work(struct work_struct *work) >> +{ >> + struct palmas_led *led = container_of(work, struct palmas_led, >> work); >> + unsigned int period, ctrl; >> + unsigned long flags; >> + >> + mutex_lock(&led->mutex); >> + >> + palmas_led_read(led->leds_data, PALMAS_LED_CTRL, &ctrl); >> + palmas_led_read(led->leds_data, PALMAS_LED_PERIOD_CTRL, &period); >> + >> + spin_lock_irqsave(&led->value_lock, flags); >> + >> + switch (led->led_no) { >> + case 1: >> + ctrl &= ~PALMAS_LED_CTRL_LED_1_ON_TIME_MASK; >> + period &= ~PALMAS_LED_PERIOD_CTRL_LED_1_PERIOD_MASK; >> + > > case 1,2,3 and 4 looks same, jus shift/mask are changed. I think this > can be rewritten here to reduce code size. Will do. > > >> +static int palmas_leds_probe(struct platform_device *pdev) >> +{ >> + struct palmas *palmas = dev_get_drvdata(pdev->dev.parent); >> + struct palmas_leds_platform_data *pdata = >> pdev->dev.platform_data; >> + struct palmas_leds_data *leds_data; >> + struct device_node *node = pdev->dev.of_node; >> + int ret, i; >> + int offset = 0; >> + >> + if (!palmas->led_muxed && >> !is_palmas_charger(palmas->product_id)) { >> + dev_err(&pdev->dev, "there are no LEDs muxed\n"); >> + return -EINVAL; >> + } >> + >> + /* Palmas charger requires platform data */ >> + if (is_palmas_charger(palmas->product_id) && node && !pdata) { >> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), >> GFP_KERNEL); >> + >> + if (!pdata) >> + return -ENOMEM; >> + >> + ret = palmas_dt_to_pdata(&pdev->dev, node, pdata); >> + if (ret) >> + return ret; >> + } >> + >> + if (is_palmas_charger(palmas->product_id) && !pdata) >> + return -EINVAL; >> + >> + leds_data = kzalloc(sizeof(*leds_data), GFP_KERNEL); > > Use devm_kzalloc(). Noted. > > >> + >> +static int palmas_leds_init(void) >> +{ >> + return platform_driver_register(&palmas_leds_driver); >> +} >> +module_init(palmas_leds_init); >> + >> +static void palmas_leds_exit(void) >> +{ >> + platform_driver_unregister(&palmas_leds_driver); >> +} >> +module_exit(palmas_leds_exit); > > use Module_platform_driver(). Will do. > Thanks, Ian