Messages in this thread | | | Date | Mon, 11 Apr 2022 10:17:14 -0500 | Subject | Re: [PATCH v2 1/2] leds: pca955x: Clean up and optimize | From | Eddie James <> |
| |
On 4/8/22 06:11, Andy Shevchenko wrote: > On Thu, Apr 7, 2022 at 10:42 PM Eddie James <eajames@linux.ibm.com> wrote: >> Clean up the I2C access functions to avoid fetching the pca955x >> driver data again. Optimize the probe to do at most 4 reads and >> 4 writes of the LED selector regs, rather than 16 of each. >> Rename some functions and variables to be more consistent and >> descriptive. > Separate patch.
Ack.
> >> + dev_err(&pca955x->client->dev, >> + "%s: reg 0x%x, val 0x%x, err %d\n", __func__, n, val, >> + ret); > This can be indented better. > > I would add a temporary dev pointer variable and put this on one line. > > struct device *dev = &pca955x->client->dev; > > dev_err(dev, "%s: reg 0x%x, val 0x%x, err %d\n", > __func__, n, val, ret);
Hm, I still end up having to use two lines for the dev_err function, so not sure it's worth it?
> > ... > >> + dev_err(&pca955x->client->dev, >> + "%s: reg 0x%x, val 0x%x, err %d\n", __func__, n, val, >> + ret); > Ditto. > > ... > >> + dev_err(&pca955x->client->dev, >> + "%s: reg 0x%x, val 0x%x, err %d\n", __func__, n, val, >> + ret); > Ditto. > > ... > >> + dev_err(&pca955x->client->dev, "%s: reg 0x%x, err %d\n", >> __func__, n, ret); > Ditto. > > ... > >> + dev_err(&pca955x->client->dev, "%s: reg 0x%x, err %d\n", >> __func__, n, ret); > Ditto. > > ... > >> + struct pca955x_led *pca955x_led = container_of(led_cdev, >> + struct pca955x_led, >> + led_cdev); > Is it used once? If more than once, consider a helper for that as well.
Ack.
Thanks for the review!
Eddie
>
| |