linux-kernel - Re: [PATCHv2 1/4] mfd: tps65912: Add new mfd device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <BANLkTi=X23M+8_qDc5N25dVSfnuwWAPXUQ@mail.gmail.com>
Date:	Fri, 13 May 2011 15:53:51 -0500
From:	Margarita Olaya <magi@...mlogic.co.uk>
To:	Jack Stone <jwjstone@...tmail.fm>
Cc:	linux-kernel@...r.kernel.org, Liam Girdwood <lrg@...mlogic.co.uk>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	sameo@...ux.intel.com
Subject: Re: [PATCHv2 1/4] mfd: tps65912: Add new mfd device

Hi Jack,

On Thu, May 12, 2011 at 2:46 PM, Jack Stone <jwjstone@...tmail.fm> wrote:
> Hi Margarita,
>
> Just a few comments.
>
> On 12/05/2011 19:42, Margarita Olaya wrote:
> [snip]
>
>> +#if defined(CONFIG_SPI_MASTER)
>> +static int tps65912_spi_write(struct tps65912 *tps65912, u8 addr,
>> +                                                     int bytes, void *src)
>> +{
>> +     struct spi_device *spi = tps65912->spi_device;
>> +     u8 *data = (u8 *) src;
>> +     int ret;
>> +
>> +     unsigned long spi_data = 1 << 23 | addr << 15 | *data;
>> +     struct spi_transfer xfer;
>> +     struct spi_message msg;
>> +     u32 tx_buf = 0, rx_buf = 0;
> These are initialized below. Can we have a blank line here please.
I will fix.

>> +     tx_buf = spi_data;
>> +     rx_buf = 0;
>> +
>> +     xfer.tx_buf     = &tx_buf;
>> +     xfer.rx_buf     = NULL;
>> +     xfer.len        = sizeof(unsigned long);
>> +     xfer.bits_per_word = 24;
>> +
>> +     spi_message_init(&msg);
>> +     spi_message_add_tail(&xfer, &msg);
>> +
>> +     ret = spi_sync(spi, &msg);
>> +     return ret;
>> +}
>> +
>> +static int tps65912_spi_read(struct tps65912 *tps65912, u8 addr,
>> +                                                     int bytes, void *dest)
>> +{
>> +     struct spi_device *spi = tps65912->spi_device;
>> +
>> +     unsigned long spi_data = 0 << 23 | addr << 15;
>
> Is the 0 << 23 meant to be 1 << 23? Like the write function.
>
No, It should be zero, I will remove.

>> +     struct spi_transfer xfer;
>> +     struct spi_message msg;
>> +     int ret;
>> +     u8 *data = (u8 *) dest;
> Shouldn't need to cast a void *
>
>> +     u32 tx_buf = 0, rx_buf = 0;
> These are initialized below.
>
>> +     tx_buf = spi_data;
>> +     rx_buf = 0;
>> +
>> +     xfer.tx_buf     = &tx_buf;
>> +     xfer.rx_buf     = &rx_buf;
>> +     xfer.len        = sizeof(unsigned long);
>> +     xfer.bits_per_word = 24;
>> +
>> +     spi_message_init(&msg);
>> +     spi_message_add_tail(&xfer, &msg);
>> +
>> +     if (spi == NULL)
>> +             return 0;
>> +
>> +     ret = spi_sync(spi, &msg);
>> +     if (ret == 0)
>> +             *data = (u8) (rx_buf & 0xFF);
>> +     return ret;
>> +}
>> +
> The spi read/write functions both ignore the bytes argument and only
> transfer one byte, whereas the i2c versions appear to read/write
> multiple bytes. Which one is correct?
>
Both are correct, I'm passing bytes argument to spi read/write to make
it compatible with the declaration of tps65912_read/write functions
pointer.

>
> I'm confused about init_data in this function.
>> +static int __devinit tps65912_spi_probe(struct spi_device *spi)
>> +{
>> +     struct tps65912 *tps65912;
>> +     struct tps65912_platform_data *init_data;
>> +     int dcdc_avs, value, ret = 0;
>> +
>> +     init_data = kzalloc(sizeof(struct tps65912_platform_data),
>> +                                                             GFP_KERNEL);
>> +     if (init_data == NULL)
>> +             return -ENOMEM;
>
> We zero allocate it here
>
>> +     tps65912 = kzalloc(sizeof(struct tps65912), GFP_KERNEL);
>> +     if (tps65912 == NULL)
>> +             return -ENOMEM;
>> +
>> +     tps65912->dev = &spi->dev;
>> +     tps65912->spi_device = spi;
>> +     tps65912->read = tps65912_spi_read;
>> +     tps65912->write = tps65912_spi_write;
>> +     mutex_init(&tps65912->io_mutex);
>> +
>> +     dcdc_avs = (init_data->is_dcdc1_avs << 0 |
>> +                             init_data->is_dcdc2_avs  << 1 |
>> +                                     init_data->is_dcdc3_avs << 2 |
>> +                                             init_data->is_dcdc4_avs << 3);
>
> And then use it here so dcdc_avs is guaranteed to be zero.
>
>> +     if (dcdc_avs) {
>> +             tps65912->read(tps65912, TPS65912_I2C_SPI_CFG, 1, &value);
>> +             dcdc_avs |= value;
>> +             tps65912->write(tps65912, TPS65912_I2C_SPI_CFG, 1, &dcdc_avs);
>> +     }
> Which means that this code will never run.
>
I will fix.

>> +
>> +     spi_set_drvdata(spi, tps65912);
>> +     ret = mfd_add_devices(tps65912->dev, -1,
>> +                           tps65912s, ARRAY_SIZE(tps65912s),
>> +                           NULL, 0);
>> +     if (ret < 0)
>> +             goto err;
>> +
>> +     return ret;
>> +
>> +err:
>> +     mfd_remove_devices(tps65912->dev);
>> +     kfree(tps65912);
>> +     return ret;
>> +}
>
> And as far as I can see nothing ever frees it.
>
I'll add kfree(init_data)

Regards,
Margarita
> Also this is not present in the i2c version of this function...
>
> Thanks,
>
> Jack
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/