On Tue, 12 May 2015, Dmitry Eremin-Solenikov wrote:
> 2015-04-28 21:45 GMT+03:00 Lee Jones <lee.jones@xxxxxxxxxx>:
> > On Tue, 28 Apr 2015, Dmitry Eremin-Solenikov wrote:
> >
> >> LoCoMo is a GA used on Sharp Zaurus SL-5x00. Current driver does has
> >> several design issues (special bus instead of platform bus, doesn't use
> >> mfd-core, etc).
> >>
> >> Implement 'core' parts of locomo support as an mfd driver.
> >>
> >> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@xxxxxxxxx>
> >> ---
>
> Thanks for the review. I agree (and have implemented) with most of
> your comments.
> However I have few questions. See below.
>
> >
> >> +/* the following is the overall data for the locomo chip */
> >> +struct locomo {
> >> + struct device *dev;
> >> + unsigned int irq;
> >> + spinlock_t lock;
> >> + struct irq_domain *domain;
> >> + struct regmap *regmap;
> >> +};
> >> +
> >> +static struct resource locomo_kbd_resources[] = {
> >> + DEFINE_RES_IRQ(IRQ_LOCOMO_KEY),
> >> +};
> >> +
> >> +static struct resource locomo_gpio_resources[] = {
> >> + DEFINE_RES_IRQ(IRQ_LOCOMO_GPIO),
> >> +};
> >> +
> >> +/* Filled in locomo_probe() function. */
> >> +static struct locomo_gpio_platform_data locomo_gpio_pdata;
> >
> > I'd prefer you didn't use globals for this.
>
> Just for platform data, or for all the structures?
Just for this. The remainder are standard.
> >> +static struct resource locomo_lt_resources[] = {
> >> + DEFINE_RES_IRQ(IRQ_LOCOMO_LT),
> >> +};
> >> +
> >> +static struct resource locomo_spi_resources[] = {
> >> + DEFINE_RES_IRQ(IRQ_LOCOMO_SPI),
> >> +};
> >> +
> >> +/* Filled in locomo_probe() function. */
> >> +static struct locomo_lcd_platform_data locomo_lcd_pdata;
> >> +
> >> +static struct mfd_cell locomo_cells[] = {
> >> + {
> >> + .name = "locomo-kbd",
> >> + .resources = locomo_kbd_resources,
> >> + .num_resources = ARRAY_SIZE(locomo_kbd_resources),
> >> + },
> >> + {
> >> + .name = "locomo-gpio",
> >> + .resources = locomo_gpio_resources,
> >> + .num_resources = ARRAY_SIZE(locomo_gpio_resources),
> >> + .platform_data = &locomo_gpio_pdata,
> >> + .pdata_size = sizeof(locomo_gpio_pdata),
> >> + },
> >> + {
> >> + .name = "locomo-lt", /* Long time timer */
> >> + .resources = locomo_lt_resources,
> >> + .num_resources = ARRAY_SIZE(locomo_lt_resources),
> >> + },
> >> + {
> >> + .name = "locomo-spi",
> >> + .resources = locomo_spi_resources,
> >> + .num_resources = ARRAY_SIZE(locomo_spi_resources),
> >> + },
> >> + {
> >> + .name = "locomo-led",
> >> + },
> >> + {
> >> + .name = "locomo-backlight",
> >> + },
> >
> > Please make these:
> >
> >> + { .name = "locomo-led" },
> >> + { .name = "locomo-backlight" },
> >
> > ... and put them at the bottom.
>
> They will be populated by of_compatible lines, so it makes little sense
> to me. What about adding of compatibility lines to this patch?
Also fine.
Although if you assure me you will do it, you can add them separately.
> >> + while (1) {
> >> + regmap_read(lchip->regmap, LOCOMO_ICR, &req);
> >> + req &= 0x0f00;
> >
> > What is this magic number? Please #define it.
>
> Adding comments to this function instead.
Also acceptable.
> >> + if (!req)
> >> + break;
> >> +
> >> + irq = ffs(req) - 9;
> >
> > Minus another random number? Either define it or enter a comment.
> >
> >> +#ifdef CONFIG_PM_SLEEP
> >> +static int locomo_suspend(struct device *dev)
> >> +{
> >> + struct locomo *lchip = dev_get_drvdata(dev);
> >> +
> >> + /* AUDIO */
> >
> > WHY ARE YOU SHOUTING? Ironic eh? ;)
> >
> >> + regmap_write(lchip->regmap, LOCOMO_PAIF, 0x00);
> >> +
> >> + /*
> >> + * Original code disabled the clock depending on leds settings
> >> + * However we disable leds before suspend, thus it's safe
> >> + * to just assume this setting.
> >> + */
> >> + /* CLK32 off */
> >> + regmap_write(lchip->regmap, LOCOMO_C32K, 0x00);
> >> +
> >> + /* 22MHz/24MHz clock off */
> >> + regmap_write(lchip->regmap, LOCOMO_ACC, 0x00);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int locomo_resume(struct device *dev)
> >> +{
> >> + struct locomo *lchip = dev_get_drvdata(dev);
> >
> > Do audio and clk sort themselves out?
>
> PAIF and ACC registers are used only by audio parts of the device. However
> there is no current Linux driver for those parts. The registers are cleared
> in case the firmware has set something in them, but in future it will
> be the task
> of the audio driver to properly clear and restore them.
>
> >> + regmap_write(lchip->regmap, LOCOMO_C32K, 0x00);
> >> +
> >> + return 0;
> >> +}
> >> +
>
> [skipped]
> >> +
> >> + if (pdata) {
> >> + locomo_gpio_pdata.gpio_base = pdata->gpio_base;
> >> + locomo_lcd_pdata.comadj = pdata->comadj;
> >> + } else {
> >> + locomo_gpio_pdata.gpio_base = -1;
> >> + locomo_lcd_pdata.comadj = 128;
> >> + }
> >
> > struct locomo_gpio_platform_data locomo_gpio_pdata;
> >
> > locomo_gpio_pdata = devm_kzalloc(<blah>);
> >
> > locomo_cells[GPIO].platform_data = locomo_gpio_pdata;
>
> I do not quite agree with you at this place. The passed platform_data
> will be kmemdup()'ed inside platform core. So the whole struct will be
> duplicated twice inside kmallocate'd memory. Ideally I'd like to drop
> the whole platform_data busyness, but that requires switching to DTS
> first.
Sounds reasonable.
> >> diff --git a/include/linux/mfd/locomo.h b/include/linux/mfd/locomo.h
> >> new file mode 100644
> >> index 0000000..6729767
> >> --- /dev/null
> >> +++ b/include/linux/mfd/locomo.h
>
> >> +/* MCS decoder for boot selecting */
> >> +#define LOCOMO_MCSX0 0x10
> >> +#define LOCOMO_MCSX1 0x14
> >> +#define LOCOMO_MCSX2 0x18
> >> +#define LOCOMO_MCSX3 0x1c
> >
> > These are pretty cryptic. Any way of making them easier to identify.
>
> No way. The names are based on old Sharp code. The drivers do not use
> them, but I'd like to still keep the registers for the reference purposes.
So they are not used at all? Then why do you want to keep them?
> >> +struct locomo_gpio_platform_data {
> >> + unsigned int gpio_base;
> >> +};
> >
> > A struct for a single int seems overkill.
> >
> >> +struct locomo_lcd_platform_data {
> >> + u8 comadj;
> >> +};
> >> +
> >> +struct locomo_platform_data {
> >> + unsigned int gpio_base;
> >> + u8 comadj;
> >> +};
> >
> > Why do you need to pass gpio_base twice?
>
> First: machine file -> core driver
> Second: core driver -> gpio driver
>
> The other way to do the same would be:
>
> struct locomo_gpio_platform_data {
> unsigned int gpio_base;
> };
>
> struct locomo_lcd_platform_data {
> u8 comadj;
> };
>
> struct locomo_platform_data {
> struct locomo_gpio_platform_data gpio_pdata;
> struct locomo_lcd_platform_data lcd_pdata;
> };
>
> And to assign pointers to the passed data in the mfd_cells
> during locomo_probe. Does that look better to you?
Bingo.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
