> +static bool oa_tc6_get_parity(u32 p)
> +{
> + bool parity = true;
> +
> + /* This function returns an odd parity bit */
> + while (p) {
> + parity = !parity;
> + p = p & (p - 1);
> + }
> + return parity;
Please take a look around and see if you can find another
implementation in the kernel which can be used. If not, you could copy/paste:
https://elixir.bootlin.com/linux/latest/source/lib/bch.c#L348
which is probably more efficient.
> +static void oa_tc6_prepare_ctrl_buf(struct oa_tc6 *tc6, u32 addr, u32 val[],
> + u8 len, bool wnr, u8 *buf, bool ctrl_prot)
> +{
> + u32 hdr;
> +
> + /* Prepare the control header with the required details */
> + hdr = FIELD_PREP(CTRL_HDR_DNC, 0) |
> + FIELD_PREP(CTRL_HDR_WNR, wnr) |
> + FIELD_PREP(CTRL_HDR_AID, 0) |
> + FIELD_PREP(CTRL_HDR_MMS, addr >> 16) |
> + FIELD_PREP(CTRL_HDR_ADDR, addr) |
> + FIELD_PREP(CTRL_HDR_LEN, len - 1);
> + hdr |= FIELD_PREP(CTRL_HDR_P, oa_tc6_get_parity(hdr));
> + *(u32 *)buf = cpu_to_be32(hdr);
> +
> + if (wnr) {
What does wnr mean? Maybe give it a more meaningful name, unless it is
actually something in the standard. Kerneldoc would also help.
> +static int oa_tc6_check_control(struct oa_tc6 *tc6, u8 *ptx, u8 *prx, u8 len,
> + bool wnr, bool ctrl_prot)
> +{
> + /* 1st 4 bytes of rx chunk data can be discarded */
> + u32 rx_hdr = *(u32 *)&prx[TC6_HDR_SIZE];
> + u32 tx_hdr = *(u32 *)ptx;
> + u32 rx_data_complement;
> + u32 tx_data;
> + u32 rx_data;
> + u16 pos1;
> + u16 pos2;
> +
> + /* If tx hdr and echoed hdr are not equal then there might be an issue
> + * with the connection between SPI host and MAC-PHY. Here this case is
> + * considered as MAC-PHY is not connected.
I could understand ENODEV on the first transaction during probe. But
after that -EIO seems more appropriate. I've also seen USB use -EPROTO
to indicate a protocol error, which a corrupt message would be.
> +int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len,
> + bool wnr, bool ctrl_prot)
> +{
> + u8 *tx_buf;
> + u8 *rx_buf;
> + u16 size;
> + u16 pos;
> + int ret;
> +
> + if (ctrl_prot)
> + size = (TC6_HDR_SIZE * 2) + (len * (TC6_HDR_SIZE * 2));
> + else
> + size = (TC6_HDR_SIZE * 2) + (len * TC6_HDR_SIZE);
Do you have an idea how big the biggest control message is? Rather
than allocate these buffers for every transaction, maybe allocate
maximum size buffers one at startup and keep them in tc6? That will
reduce overhead and simplify the code.
> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
> +{
> + struct oa_tc6 *tc6;
> +
> + if (!spi)
> + return NULL;
This is defensive programming which is generally not liked. You cannot
do anything without an SPI device, so just assume it is passed, and if
not, let is explode later and the driver write will quickly fix there
broken code.
> +
> + tc6 = kzalloc(sizeof(*tc6), GFP_KERNEL);
> + if (!tc6)
> + return NULL;
> +
> + tc6->spi = spi;
> +
> + return tc6;
> +}
> +EXPORT_SYMBOL_GPL(oa_tc6_init);
> +
> +void oa_tc6_deinit(struct oa_tc6 *tc6)
> +{
> + kfree(tc6);
> +}
> +EXPORT_SYMBOL_GPL(oa_tc6_deinit);
Maybe consider a devm_ API to make the MAC driver simpler.
Andrew
Hi Andrew,
On 13/09/23 7:41 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> +static bool oa_tc6_get_parity(u32 p)
>> +{
>> + bool parity = true;
>> +
>> + /* This function returns an odd parity bit */
>> + while (p) {
>> + parity = !parity;
>> + p = p & (p - 1);
>> + }
>> + return parity;
>
> Please take a look around and see if you can find another
> implementation in the kernel which can be used. If not, you could copy/paste:
>
> https://elixir.bootlin.com/linux/latest/source/lib/bch.c#L348
>
> which is probably more efficient.
Sure, will check out it.
>
>> +static void oa_tc6_prepare_ctrl_buf(struct oa_tc6 *tc6, u32 addr, u32 val[],
>> + u8 len, bool wnr, u8 *buf, bool ctrl_prot)
>> +{
>> + u32 hdr;
>> +
>> + /* Prepare the control header with the required details */
>> + hdr = FIELD_PREP(CTRL_HDR_DNC, 0) |
>> + FIELD_PREP(CTRL_HDR_WNR, wnr) |
>> + FIELD_PREP(CTRL_HDR_AID, 0) |
>> + FIELD_PREP(CTRL_HDR_MMS, addr >> 16) |
>> + FIELD_PREP(CTRL_HDR_ADDR, addr) |
>> + FIELD_PREP(CTRL_HDR_LEN, len - 1);
>> + hdr |= FIELD_PREP(CTRL_HDR_P, oa_tc6_get_parity(hdr));
>> + *(u32 *)buf = cpu_to_be32(hdr);
>> +
>> + if (wnr) {
>
> What does wnr mean? Maybe give it a more meaningful name, unless it is
> actually something in the standard. Kerneldoc would also help.
Ah, it is "write not read". Shall I name it as "write_not_read" ?
>
>> +static int oa_tc6_check_control(struct oa_tc6 *tc6, u8 *ptx, u8 *prx, u8 len,
>> + bool wnr, bool ctrl_prot)
>> +{
>> + /* 1st 4 bytes of rx chunk data can be discarded */
>> + u32 rx_hdr = *(u32 *)&prx[TC6_HDR_SIZE];
>> + u32 tx_hdr = *(u32 *)ptx;
>> + u32 rx_data_complement;
>> + u32 tx_data;
>> + u32 rx_data;
>> + u16 pos1;
>> + u16 pos2;
>> +
>> + /* If tx hdr and echoed hdr are not equal then there might be an issue
>> + * with the connection between SPI host and MAC-PHY. Here this case is
>> + * considered as MAC-PHY is not connected.
>
> I could understand ENODEV on the first transaction during probe. But
> after that -EIO seems more appropriate. I've also seen USB use -EPROTO
> to indicate a protocol error, which a corrupt message would be.
Ah ok, then in this case I will consider -EIO.
>
>> +int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len,
>> + bool wnr, bool ctrl_prot)
>> +{
>> + u8 *tx_buf;
>> + u8 *rx_buf;
>> + u16 size;
>> + u16 pos;
>> + int ret;
>> +
>> + if (ctrl_prot)
>> + size = (TC6_HDR_SIZE * 2) + (len * (TC6_HDR_SIZE * 2));
>> + else
>> + size = (TC6_HDR_SIZE * 2) + (len * TC6_HDR_SIZE);
>
> Do you have an idea how big the biggest control message is? Rather
> than allocate these buffers for every transaction, maybe allocate
> maximum size buffers one at startup and keep them in tc6? That will
> reduce overhead and simplify the code.
Ok, as per OA spec, up to 128 consecutive registers read or write can be
possible. So the maximum possible size would be 1032. As you suggested
will allocate this size of memory in the startup.
>
>> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
>> +{
>> + struct oa_tc6 *tc6;
>> +
>> + if (!spi)
>> + return NULL;
>
> This is defensive programming which is generally not liked. You cannot
> do anything without an SPI device, so just assume it is passed, and if
> not, let is explode later and the driver write will quickly fix there
> broken code.
Ah yes, will remove this check.
>
>> +
>> + tc6 = kzalloc(sizeof(*tc6), GFP_KERNEL);
>> + if (!tc6)
>> + return NULL;
>> +
>> + tc6->spi = spi;
>> +
>> + return tc6;
>> +}
>> +EXPORT_SYMBOL_GPL(oa_tc6_init);
>> +
>> +void oa_tc6_deinit(struct oa_tc6 *tc6)
>> +{
>> + kfree(tc6);
>> +}
>> +EXPORT_SYMBOL_GPL(oa_tc6_deinit);
>
> Maybe consider a devm_ API to make the MAC driver simpler.
Sorry I don't get your point. Could you please explain bit more?
Best Regards,
Parthiban V
>
> Andrew
>
> >> +static void oa_tc6_prepare_ctrl_buf(struct oa_tc6 *tc6, u32 addr, u32 val[],
> >> + u8 len, bool wnr, u8 *buf, bool ctrl_prot)
> >> +{
> >> + u32 hdr;
> >> +
> >> + /* Prepare the control header with the required details */
> >> + hdr = FIELD_PREP(CTRL_HDR_DNC, 0) |
> >> + FIELD_PREP(CTRL_HDR_WNR, wnr) |
> >> + FIELD_PREP(CTRL_HDR_AID, 0) |
> >> + FIELD_PREP(CTRL_HDR_MMS, addr >> 16) |
> >> + FIELD_PREP(CTRL_HDR_ADDR, addr) |
> >> + FIELD_PREP(CTRL_HDR_LEN, len - 1);
> >> + hdr |= FIELD_PREP(CTRL_HDR_P, oa_tc6_get_parity(hdr));
> >> + *(u32 *)buf = cpu_to_be32(hdr);
> >> +
> >> + if (wnr) {
> >
> > What does wnr mean? Maybe give it a more meaningful name, unless it is
> > actually something in the standard. Kerneldoc would also help.
> Ah, it is "write not read". Shall I name it as "write_not_read" ?
You might want to describe the high level concept as well in this
file. What i _think_ this is about is that SPI is sort of a full
duplex bus. While you are sending data to the SPI device, the device
could also be sending a data to the CPU? And 'write not read' here
means ignore what we receive from the device?
> Ok, as per OA spec, up to 128 consecutive registers read or write can be
> possible. So the maximum possible size would be 1032. As you suggested
> will allocate this size of memory in the startup.
Yes, 1032 bytes it not huge, so allocate it once and keep it for the
lifetime of the device.
> >> +void oa_tc6_deinit(struct oa_tc6 *tc6)
> >> +{
> >> + kfree(tc6);
> >> +}
> >> +EXPORT_SYMBOL_GPL(oa_tc6_deinit);
> >
> > Maybe consider a devm_ API to make the MAC driver simpler.
> Sorry I don't get your point. Could you please explain bit more?
At least at this stage in the patch series, all you are doing is
allocating memory. You add more code later, which might invalidate my
point. But if all you are doing is allocating memory, you could use
devm_kmalloc(). The driver core will then take care of releasing the
memory when the driver is unloaded, or probe fails. That makes cleanup
simpler and memory leaks less likely. There are a lot of devm_
helpers, see if you can use them.
Andrew
Hi Andrew,
On 19/09/23 8:43 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>>>> +static void oa_tc6_prepare_ctrl_buf(struct oa_tc6 *tc6, u32 addr, u32 val[],
>>>> + u8 len, bool wnr, u8 *buf, bool ctrl_prot)
>>>> +{
>>>> + u32 hdr;
>>>> +
>>>> + /* Prepare the control header with the required details */
>>>> + hdr = FIELD_PREP(CTRL_HDR_DNC, 0) |
>>>> + FIELD_PREP(CTRL_HDR_WNR, wnr) |
>>>> + FIELD_PREP(CTRL_HDR_AID, 0) |
>>>> + FIELD_PREP(CTRL_HDR_MMS, addr >> 16) |
>>>> + FIELD_PREP(CTRL_HDR_ADDR, addr) |
>>>> + FIELD_PREP(CTRL_HDR_LEN, len - 1);
>>>> + hdr |= FIELD_PREP(CTRL_HDR_P, oa_tc6_get_parity(hdr));
>>>> + *(u32 *)buf = cpu_to_be32(hdr);
>>>> +
>>>> + if (wnr) {
>>>
>>> What does wnr mean? Maybe give it a more meaningful name, unless it is
>>> actually something in the standard. Kerneldoc would also help.
>> Ah, it is "write not read". Shall I name it as "write_not_read" ?
>
> You might want to describe the high level concept as well in this
> file. What i _think_ this is about is that SPI is sort of a full
> duplex bus. While you are sending data to the SPI device, the device
> could also be sending a data to the CPU? And 'write not read' here
> means ignore what we receive from the device?
Ah ok, I think there is a misunderstanding here. This is related to OPEN
Alliance protocol. Control transactions consist of one or more control
commands. Control commands are used by the SPI host to read and write
registers within the MAC-PHY. Each control commands are composed of a
32-bit control command header followed by register data. WNR (write not
read) bit in the control command header indicates if data is to be
written to registers (when set) or read from registers (when clear). so
basically it indicates the type of the control command on the registers.
>
>> Ok, as per OA spec, up to 128 consecutive registers read or write can be
>> possible. So the maximum possible size would be 1032. As you suggested
>> will allocate this size of memory in the startup.
>
> Yes, 1032 bytes it not huge, so allocate it once and keep it for the
> lifetime of the device.
Sure, will do that.
>
>>>> +void oa_tc6_deinit(struct oa_tc6 *tc6)
>>>> +{
>>>> + kfree(tc6);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(oa_tc6_deinit);
>>>
>>> Maybe consider a devm_ API to make the MAC driver simpler.
>> Sorry I don't get your point. Could you please explain bit more?
>
> At least at this stage in the patch series, all you are doing is
> allocating memory. You add more code later, which might invalidate my
> point. But if all you are doing is allocating memory, you could use
> devm_kmalloc(). The driver core will then take care of releasing the
> memory when the driver is unloaded, or probe fails. That makes cleanup
> simpler and memory leaks less likely. There are a lot of devm_
> helpers, see if you can use them.
Sure, noted.
Best Regards,
Parthiban V
>
> Andrew
>
> Ah ok, I think there is a misunderstanding here. This is related to OPEN
> Alliance protocol. Control transactions consist of one or more control
> commands. Control commands are used by the SPI host to read and write
> registers within the MAC-PHY. Each control commands are composed of a
> 32-bit control command header followed by register data. WNR (write not
> read) bit in the control command header indicates if data is to be
> written to registers (when set) or read from registers (when clear). so
> basically it indicates the type of the control command on the registers.
OK, so this clearly indicates the names are bad and documentation is
missing if i got this completely wrong. Adding kerneldoc to these
functions should help.
Andrew
Hi Andrew,
On 20/09/23 7:07 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> Ah ok, I think there is a misunderstanding here. This is related to OPEN
>> Alliance protocol. Control transactions consist of one or more control
>> commands. Control commands are used by the SPI host to read and write
>> registers within the MAC-PHY. Each control commands are composed of a
>> 32-bit control command header followed by register data. WNR (write not
>> read) bit in the control command header indicates if data is to be
>> written to registers (when set) or read from registers (when clear). so
>> basically it indicates the type of the control command on the registers.
>
> OK, so this clearly indicates the names are bad and documentation is
> missing if i got this completely wrong. Adding kerneldoc to these
> functions should help.
Sure, I will do that.
>
> Andrew
>
>