Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c96c5554-d2ae-33a4-ba1b-8c76998e0367@traphandler.com>
Date:   Tue, 14 Jun 2022 14:52:49 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v3 2/3] leds: Add driver for the TLC5925 LED controller
Content-Language: en-US
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
CC:     Pavel Machek <pavel@ucw.cz>, <krzk+dt@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Linux LED Subsystem <linux-leds@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <20220609162734.1462625-1-jjhiblot@traphandler.com>
 <20220609162734.1462625-3-jjhiblot@traphandler.com>
 <CAHp75Veurvhxi0Pg1Sjxav+3XpDTVOdan8WFFmZmdhJbZJiCaQ@mail.gmail.com>
From:   Jean-Jacques Hiblot <jjhiblot@traphandler.com>
In-Reply-To: <CAHp75Veurvhxi0Pg1Sjxav+3XpDTVOdan8WFFmZmdhJbZJiCaQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 09/06/2022 18:57, Andy Shevchenko wrote:
> On Thu, Jun 9, 2022 at 6:30 PM Jean-Jacques Hiblot
> <jjhiblot@traphandler.com> wrote:
>> The TLC5925 is a 16-channels constant-current LED sink driver.
>> It is controlled via SPI but doesn't offer a register-based interface.
>> Instead it contains a shift register and latches that convert the
>> serial input into a parallel output.
> Can you add Datasheet: tag here with the corresponding URL? Rationale
> is to get a link to the datasheet by just browsing Git log without
> browsing the source code, which will benefit via Web UIs.
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> ...
>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/leds.h>
>> +#include <linux/err.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/property.h>
>> +#include <linux/workqueue.h>
> Keep it sorted?
>
> ...
>
>> +struct single_led_priv {
>> +       int idx;
>> +       struct led_classdev cdev;
> For pointer arithmetics it's better to swap these two members.
>
>> +};
>> +
>> +struct tlc5925_leds_priv {
>> +       int max_num_leds;
>> +       u8 *state;
> unsigned long? DECLARE_BITMAP() ?
>
>> +       spinlock_t lock;
>> +       struct single_led_priv leds[];
>> +};
> ...
>
>> +       if (brightness)
>> +               priv->state[index / 8] |= (1 << (index % 8));
>> +       else
>> +               priv->state[index / 8] &= ~(1 << (index % 8));
> assign_bit()
>
> ...
>
>> +       return spi_write(spi, priv->state, priv->max_num_leds / 8);
> BITS_TO_BYTES() ?
>
> ...
>
>> +       count = device_get_child_node_count(dev);
>> +       if (!count) {
>> +               dev_err(dev, "no led defined.\n");
>> +               return -ENODEV;
>    return dev_err_probe(...);
>
> here and everywhere in ->probe() and Co.
>
>> +       }
> ...
>
>> +       ret = device_property_read_u32_array(dev, "ti,shift-register-length",
>> +                                            &max_num_leds, 1);
> Always an array of 1 element? call device_property_read_u32().
>
> ...
>
>> +       if (max_num_leds % 8) {
>> +               dev_err(dev, "'ti,shift-register-length' must be a multiple of 8\n");
>> +               return -EINVAL;
>> +       }
> Is this really fatal? I would rather issue a warning and go on if it
> has at least 8 there. So the idea is to use a minimum that holds
> multiple of 8.
It is more than likely that it will always be a multiple of 8 here.

We could in theory use a non-multiple of 8 (some LEDs of the first or 
last chips of the chain are not populated).

I didn't think it would add a significant benefit in memory usage. In 
terms of SPI usage it wouldn't change anything.


Thanks for your feedback.

> ...
>
>> +       /* Assert all the OE/ lines */
>> +       gpios = devm_gpiod_get_array(dev, "output-enable-b", GPIOD_OUT_LOW);
>> +       if (IS_ERR(gpios)) {
>> +               dev_err(dev, "Unable to get the 'output-enable-b' gpios\n");
>> +               return PTR_ERR(gpios);
>> +       }
> You have to use dev_err_probe() here, otherwise it will spam logs a
> lot in case of deferred probe.
>
> ...
>
>> +       priv->state = devm_kzalloc(dev, DIV_ROUND_UP(max_num_leds, 8), GFP_KERNEL);
> devm_bitmap_zalloc()
>
> ...
>
>> +       device_for_each_child_node(dev, child) {
>> +               struct led_init_data init_data = {.fwnode = child};
> Missed spaces.
>
>> +               struct led_classdev *cdev;
>> +               u32 idx;
>> +
>> +               ret = fwnode_property_read_u32_array(child, "reg", &idx, 1);
> fwnode_property_read_u32()
>
>> +               if (ret || idx >= max_num_leds) {
>> +                       dev_err(dev, "%s: invalid reg value. Ignoring.\n",
>> +                               fwnode_get_name(child));
>> +                       fwnode_handle_put(child);
>> +                       continue;
> Either dev_warn + continue, or dev_err + return dev_err_probe().
>
>> +               }
>> +
>> +               count--;
>> +               priv->leds[count].idx = idx;
>> +               cdev = &(priv->leds[count].cdev);
>> +               cdev->brightness = LED_OFF;
>> +               cdev->max_brightness = 1;
>> +               cdev->brightness_set_blocking = tlc5925_brightness_set_blocking;
>> +
>> +               ret = devm_led_classdev_register_ext(dev, cdev, &init_data);
>> +               if (ret) {
> Ditto.
>
>> +                       dev_err(dev, "%s: cannot create LED device.\n",
>> +                               fwnode_get_name(child));
>> +                       fwnode_handle_put(child);
>> +                       continue;
>> +               }
>> +       }
> ...
>
>> +static const struct of_device_id tlc5925_dt_ids[] = {
>> +       { .compatible = "ti,tlc5925", },
>> +       {},
> No comma for terminator entry.
>
>> +};
> Where is the MODULE_DEVICE_TABLE() for this one?
>
> ...
>
>> +
> No  need for this blank line.
>
>> +module_spi_driver(tlc5925_driver);