On 28/02/13 11:52, Ian Lartey wrote:

On 28/02/13 05:44, Laxman Dewangan wrote:

quoted

On Wednesday 27 February 2013 06:43 PM, Ian Lartey wrote:

quoted

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 <redacted>
Signed-off-by: Ian Lartey <redacted>
---

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.

quoted

quoted

+
+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.

quoted

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.

quoted

quoted

+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.

Hi Laxman,

Looking at this again I think the cose is as small as it can be
whilst maintaining readability.
Is is fairly compact as this is setting up the ctrl, period
led->blink, and led->brightness values  that are used in the
next section to actually write to the led control registers.

Ian

quoted

quoted

+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.

quoted

quoted

+
+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.

quoted

Thanks,

Ian