Re: [RFC PATCH RESEND 1/3] mfd: upboard: Add UP2 platform controller driver

From: Javier Arteaga
Date: Wed Apr 25 2018 - 22:33:51 EST


Hi Andy,

First off, many thanks for your thorough review! Replies inline.

On Wed, Apr 25, 2018 at 06:57:30PM +0300, Andy Shevchenko wrote:
> > +config MFD_UPBOARD
> > + tristate "UP Squared"
> > + depends on ACPI
> > + depends on GPIOLIB
> > + select MFD_CORE
> > + select REGMAP
> > + help
> > + If you say yes here you get support for the platform
> > controller
> > + of the UP Squared single-board computer.
> > +
> > + This driver provides common support for accessing the
> > device,
> > + additional drivers must be enabled in order to use the
> > + functionality of the device.
> > +
> > + This driver can also be built as a module. If so, the
> > module
> > + will be called upboard.
>
> "upboard"

The module name in quotes reads better to me too, but the majority of
Kconfig entries do it this way, looks like:

linux $ rg 'module will be called [^"]' | wc -l
1275
linux $ rg 'module will be called "' | wc -l
5

> > +static int upboard_read(void *context, unsigned int reg, unsigned int
> > *val)
> Can't you rewrite this like
>
> for (addr) {
> strobe(0)
> data(x)
> strobe(1)
> }
>
> for (register) {
> strobe(0)
> val = data(x)
> strobe(1)
> }
>
> val &= BIT(register_size);
> strobe(0)
>
> ?

> > +static int upboard_write(void *context, unsigned int reg, unsigned
> > int val)
> Similar here:
>
> for (addr) {
> strobe(0)
> data(x)
> strobe(1)
> }
>
> for (register) {
> strobe(0)
> data(x)
> strobe(1)
> }
>
> strobe(0)
> strobe(1)
>
> ?

> Moreover these two functions have duplications, i.e.
>
> static ... upboard_clear()
> {
> clear(0)
> clear(1)
> }
>
> static ... upboard_set_address()
> {
> for (addr) {
> ...
> }
> }

I'll look into making these R/W functions more compact.

> Additional question is about spi-bitbang and i2c-gpio. Can one of them
> be utilized here? Why not?

i2c-gpio would be closest, but unfortunately this isn't quite I2C:

- two in/out GPIOs instead of a single SDA line,
- R/W sequence start is signaled from yet _another_ line,
- ACK implicit in last rising edge of the address instead of an ACK pulse,
- etc...

Probably should explain this in a comment too.

> > +static int upboard_init_gpio(struct upboard *upboard)
> > +{
> > + struct gpio_desc *enable_gpio;
> > +
> > + enable_gpio = devm_gpiod_get(upboard->dev, "enable",
> > GPIOD_OUT_LOW);
> > + if (IS_ERR(enable_gpio))
> > + return PTR_ERR(enable_gpio);
>
> > + gpiod_set_value(enable_gpio, 1);
>
> When do you disable it? Why not?

enable = 0/1 sets all FPGA pins for FPGA-routed lines Hi-Z/active. It's
probably safer to set enable = 0 on unload.

I'll go over this again (and add comments in any case).

> > +static int upboard_check_supported(struct upboard *upboard)
> > +{
>
> > + const unsigned int AAEON_MANUFACTURER_ID = 0x01;
> > + const unsigned int SUPPORTED_FW_MAJOR = 0x0;
>
> Why to hide here instead of putting at the top of file as defined
> constants?

No strong reason. I'll move them to the top.

> > + build = (firmware_id >> 12) & 0xf;
> > + major = (firmware_id >> 8) & 0xf;
> > + minor = (firmware_id >> 4) & 0xf;
>
> > + patch = firmware_id & 0xf;
>
> For style purposes you can use
> (firmware >> 0) & 0xf here

Sure, why not.

> > +static int upboard_probe(struct platform_device *pdev)
> > +{
> > + struct upboard *upboard;
> > + const struct acpi_device_id *id;
> > + const struct upboard_data *upboard_data;
> > + int ret;
>
> > + id = acpi_match_device(upboard_acpi_match, &pdev->dev);
> > + if (!id)
> > + return -ENODEV;
> > +
> > + upboard_data = (const struct upboard_data *) id->driver_data;
>
> Use new API for that.

Will do.

> > +MODULE_LICENSE("GPL");
>
> License mismatch.

True, it should read "GPL v2". I'll update these.

> > +#define UPBOARD_ADDRESS_SIZE 7
> > +#define UPBOARD_REGISTER_SIZE 16
>
> > +#define UPBOARD_READ_FLAG BIT(UPBOARD_ADDRESS_SIZE)
>
> It's not clear why this one is defined in this way.
> Comment is needed.

It means that the RW flag comes after the last bit of the address, like
in I2C. I'll likely drop this #define and make this clearer next
iteration.